pybind11-stubgen errors

[MeetWq]

I'm trying to generate type stubs using pybind11-stubgen. However there are many errors in the generation process. Following the error message, I made some changes to the code: https://github.com/MeetWq/skia-python/tree/fix/pybind11

My changes mainly involve the following two aspects：

• split pybind declarations and definitions
  According to the document of pybind11, docstrings are generated at the time of the declaration, e.g. when .def(...) is called. At this point parameter and return types should be known to pybind11. By spliting the py::class_ constructors and .def(...) definitions, some C++ type names in the docstring were replaced:
  before:
  
  after:
  

• manually specify preview of default arguments
  According to the document of pybind11, the “preview” of the default argument in the function signature is generated using the object’s __repr__ method. If not available, the signature may not be very helpful. We can manually specify the human-readable preview of the default argument using the arg_v notation:
  before:
  
  after:
  

With the above changes, most of the errors that occurred during the execution of pybind11-stubgen have been resolved, but there are still some remaining errors:

pybind11_stubgen - [  ERROR] In skia.BBoxHierarchy.search : Invalid expression 'std::vector<int'
pybind11_stubgen - [  ERROR] In skia.BBoxHierarchy.search : Invalid expression ':allocator<int> >'
pybind11_stubgen - [  ERROR] In skia.ColorSpace.Deserialize : Can't find/import 'capsule'
pybind11_stubgen - [  ERROR] In skia.ColorSpace.writeToMemory : Can't find/import 'capsule'
pybind11_stubgen - [  ERROR] In skia.GrBackendSemaphore.MakeVk : Can't find/import 'capsule'
pybind11_stubgen - [  ERROR] In skia.GrBackendSemaphore.vkSemaphore : Can't find/import 'capsule'
pybind11_stubgen - [  ERROR] In skia.GrBackendTexture.setMutableState : Invalid expression 'skgpu::MutableTextureState'
pybind11_stubgen - [  ERROR] In skia.GrContext_Base.threadSafeProxy : Can't find/import 'GrContextThreadSafeProxy'
pybind11_stubgen - [  ERROR] In skia.GrDirectContext.setBackendRenderTargetState : Invalid expression 'skgpu::MutableTextureState'
pybind11_stubgen - [  ERROR] In skia.GrDirectContext.setBackendTextureState : Invalid expression 'skgpu::MutableTextureState'
pybind11_stubgen - [  ERROR] In skia.GrVkImageInfo.fSharingMode. : Can't find/import 'VkSharingMode'
pybind11_stubgen - [  ERROR] In skia.GrVkImageInfo.fSharingMode. : Can't find/import 'VkSharingMode'
pybind11_stubgen - [  ERROR] In skia.MemoryStream.__init__ : Can't find/import 'capsule'
pybind11_stubgen - [  ERROR] In skia.MemoryStream.getAtPos : Can't find/import 'capsule'
pybind11_stubgen - [  ERROR] In skia.Point.Offset : Invalid expression 'std::vector<SkPoint'
pybind11_stubgen - [  ERROR] In skia.Point.Offset : Invalid expression ':allocator<SkPoint> >'
pybind11_stubgen - [  ERROR] In skia.Point.Offset : Invalid expression 'std::vector<SkPoint, std::allocator<SkPoint> >'
pybind11_stubgen - [  ERROR] In skia.Shaders.Blend : Can't find/import 'SkBlender'
pybind11_stubgen - [  ERROR] In skia.Stream.getMemoryBase : Can't find/import 'capsule'
pybind11_stubgen - [  ERROR] In skia.Surface.AsyncReadResult.data : Can't find/import 'capsule'
pybind11_stubgen - [  ERROR] In skia.Surface.WrapBackendRenderTarget : Invalid expression 'void (void*)'
pybind11_stubgen - [  ERROR] In skia.Surface.WrapBackendRenderTarget : Can't find/import 'capsule'
pybind11_stubgen - [  ERROR] In skia.Surface.flush : Invalid expression 'skgpu::MutableTextureState'
pybind11_stubgen - [WARNING] Raw C++ types/values were found in signatures extracted from docstrings.
Please check the corresponding sections of pybind11 documentation to avoid common mistakes in binding code:
 - https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-cpp-types-in-docstrings
 - https://pybind11.readthedocs.io/en/latest/advanced/functions.html#default-arguments-revisited
pybind11_stubgen - [   INFO] Terminating due to previous errors

Since I'm not familiar with either pybind11 or skia, I'm not sure how to fix them. The above errors show some problems in skia-python, such as skgpu::MutableTextureState is not defined with py::class_.

Can you help with these errors? Should I contribute these changes to the repository?

[HinTak]

While I appreciate the effort, I don't think there is any error to be fixed, hence I am just going to close this. Here are my reasonings:

• the first and most important reason: we can't take large changes to existing code, on purely stylistic / philosophical grounds. If the suggested changes pass the existing tests (numbered to 2000+ at the moment), we may think about it. So if you want to suggest such changes, the first thing it needs to do, is to create a pull that passes CI (without disabling too many tests on the way). We can't take large changes and large re-organizations, based purely on stylistic/philosophical reasons. If you want to suggest such changes on beyond stylistic/philosophical grounds, adding some tests would be good.

• skia is a moving target, routines appears and disappears... not sure that automatically binding every corner of it is a good idea.

• on the specific messages. Half of it is about not knowing capsule. capsule is a block of c-memory wrapped in a python object. It is used by skia (or any c-based library) to interact with another c-based library via python, when a block of memory is wrapped and transported between two libraries (numpy, opengl, vulkan, etc, from the look of those messages). They are not error messages about skia or skia-python AFAIC. They are error messages about pybind11-stubgen not knowing capsule. At a glance, I believe those usages of capsule are correct and intentional. Please file an issue at pybind11-stubgen.

• that said, I think using pybind11-stubgen on

• new code area
• with substantial editing / cut-down / removal after auto-generation.
  is probably a good approach.

Sorry this might not be what you want to hear. Personally I would reject large changes of mostly cosmetic nature, simply because of the substantial time it requires to check that the changes are indeed all cosmetic.

cc @kyamagu

[HinTak]

Btw, there is a still-open issue #133 along the same direction. The main difference is that this issue seems to suggest substantially re-formatting existing code, which I would somewhat oppose unless there is a good reason.

[MeetWq]

Surely the changes to the code are too big to review, but the current documentation does have the issues I mentioned above, that is C++ type names in the docstring. Maybe I could try to resolve that without reformatting the code a lot.

[HinTak]

Perhaps that was a rushed response - on 2nd thought while I agree that the issues you pointed out are real and could benefit from some work, it remains a issue that large and mostly automated changes would take substantial time checking. To the extent that the amount of effort in checking may be almost the same as one of us (the known reliable contributors) running the auto-tool and doing it ourselves.

So I would say, a pull of this nature should be accompanied by a detailed description of the procedure of how to (independently) arriving at the proposed change? And ideally, none of the steps should be "editing files by hand". Would that be a better answer? I.e. one of us will still be "independently" trying to arrive at the same changes, the only difference being that you are credited as the author of the change.

This is assuming that most/all of the changes are auto-generated in nature. If it is about adjusting current code manually to fit auto-tools, my original comment stands.

[kyamagu]

@MeetWq Thanks for identifying the cause and a fix for C++ type name issues.

@HinTak While code reformatting can be a lot, I don't see much functionality change in @MeetWq 's proposal. Perhaps the process can be mostly automated as you point out.

Another concern is the conflict with the current @HinTak 's development fork.

[HinTak]

The 2nd part wasn't too painful to check, so I have simply included it in pull #258 in 5122c9d .

The first part is a bit too painful. Anyway, I tried running pybind11-stubgen - it doesn't quite do what I think it would do...

[HinTak]

I have split off the first item as #262 . The 2nd item is included in #258 ; some of the 3rd items will disappear as api comes and goes. I think there are simple improvement possible for #262, but the bulk of it is due to classes having methods involving and inter-dependent on each other, Canvas vs surface, image vs pixmap, etc, so those few files will need splitting into declarations and methods and large re-arrangements...