Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically generate typing stubs #16

Merged
merged 15 commits into from Jun 9, 2021

Conversation

Muream
Copy link
Contributor

@Muream Muream commented May 10, 2021

This PR enables generating typing stubs for cmdc automatically.

This is still WIP and I'm not really happy with the organization of everything yet, I'm open to any ideas there.

To generate the stubs:

  • run mayapy scripts/generate_typing_stubs.py
  • cmdc needs to be available in the PYTHONPATH
  • pybind11-stubgen needs to be installed in mayapy

Some notes:

  • I've had to declare all the classes before they're used anywhere to Avoiding having C++ types in the docstrings, this caused syntax errors and messed up the stubs generations.
    I've put that the forward declarations in ForwardDeclarations.inl. I don't know if there's a better way to do that.
  • I haven't automated the generation when building yet because I only have a Windows machine and don't know PowerShell at all.
  • The stubs are saved in the build directory, not sure if that's where they should be saved.
  • Some of the docstrings have typing information, I'm not sure if they're still relevant but I've left them for now in case they were.
    pybind11-stubgen does take those into account and some weren't valid ((string, string, ...) instead of List[str] ) so I've updated those.

@mottosso
Copy link
Owner

Oh wow, very nice.

I've put that the forward declarations in ForwardDeclarations.inl. I don't know if there's a better way to do that.

I think that's sensible, obvious what it is without making noise.

I haven't automated the generation when building yet because I only have a Windows machine and don't know PowerShell at all.

Everything you need should be in the existing powershell file, most of it is pretty printing things to make the logs more approachable.

If it remains a mystery, just leave it and I'll take a look at it.

The stubs are saved in the build directory, not sure if that's where they should be saved.

In order to use them, they'll need to eventually end up on PYTHONPATH right? I haven't used .pyi files before, but that would make sense. In that case, building them alongside the .pyd file makes sense, and for distribution files get copied by absolute path at which point it doesn't matter where they are so long as it's consistent.

Here's where files are extracted from each build:

path: build/cmdc.pyd

Some of the docstrings have typing information, I'm not sure if they're still relevant but I've left them for now in case they were. pybind11-stubgen does take those into account and some weren't valid ((string, string, ...) instead of List[str] ) so I've updated those.

Not sure, maybe @yantor3d knows more?

@Muream
Copy link
Contributor Author

Muream commented May 11, 2021

In order to use them, they'll need to eventually end up on PYTHONPATH right?

Yeah, they're only relevant in your editor, they don't get use at runtime at all as far as I know.
In vscode when using pylance you need to set the python.analysis.extraPaths to extend the PYTHONPATH of the editor, not sure about pycharm or other editors

@Muream
Copy link
Contributor Author

Muream commented May 11, 2021

I just added the stubs generation in build_win32.ps1.
I've also removed the python script that generates the stubs because turns out you can just call pybind11_stubgen from the command line

The checks currently fail for windows because we need to install pybind11-stubgen, not sure how to set that up with the github actions.
I've also not changed the build script for Linux.

@yantor3d
Copy link
Contributor

The docstrings in the *.inl files are scraped from the devkit headers, so any type declarations in them may not agree with the Python binding method signatures.

@Muream
Copy link
Contributor Author

Muream commented May 11, 2021

So nothing relies on them right?
It's fine if I remove them entirely?

@yantor3d
Copy link
Contributor

So nothing relies on them right?
It's fine if I remove them entirely?

Nothing relies on them. Will the stubs provide docstrings instead?

@Muream
Copy link
Contributor Author

Muream commented May 11, 2021

Ah sorry I meant removing the function signature out of the docstrings, not completely removing the docstrings.

The stubs generation relies on what's in the pyd file so it just piggy backs on the work that's been done to make the bindings. It's by no means a replacement to anything.

The issue I'm having is that if a signature is specified in the docstring, it takes over the actual signature and ends up being the one in the stubs which might lead to inconsistent and outdated signatures.

The stubs file contains 2 things:

  • The function/class signatures, with the python3 type annotation syntax
  • The docstrings, stripped of any signature

@yantor3d
Copy link
Contributor

Ah, I see what you mean. MSelectionList has some methods signatures baked into its docstrings. Only 22 though, so I feel like we can remove then by hand, and then update the parse_header script to sanitize the docstrings.

@Muream
Copy link
Contributor Author

Muream commented May 11, 2021

Yeah that sounds like a plan, I'll do that sometime this week

@Muream
Copy link
Contributor Author

Muream commented May 12, 2021

@mottosso I feel like we're getting there, I'm just not sure how to make the CICD work now

@mottosso
Copy link
Owner

Maya isn't available to CI at the moment, only regular Python with the Maya devkit. Do we need it? We'll need it to run the tests and I've been meaning to add it anyway. Could get that underway this weekend.

@Muream
Copy link
Contributor Author

Muream commented May 13, 2021

Ah I assumed it was, that tracks.

Pybind11-stubgen needs to import cmdc and as far as I know that can only work from mayapy

@mottosso
Copy link
Owner

Cool, I'll add it to CI this weekend, then you can merge that into here to test it out.

This was referenced May 16, 2021
@mottosso
Copy link
Owner

Done. It's all yours.

@Muream
Copy link
Contributor Author

Muream commented May 25, 2021

I've just added pybind11-stubgen in the "Setup Test Environment" part, I expect it to fail on everything but 2022, pybind11-stubgen seems to only be able to run from python3, not too sure how we should deal with that.

Edit: Looks like it worked on Linux, I haven't added the stubs generation there so that makes sense.
I don't have a Linux machine at hand to set that up though

Edit 2: Oooh and the bindings get built before the test environment is setup, I didn't consider that.
@mottosso I'm not familiar with github actions so if you have any recommendations on what should be done, I'm all ears

@mottosso
Copy link
Owner

I don't have a Linux machine at hand to set that up though

mayapy is only available to GitHub on Linux, so if that's required then it'll only ever work on Linux.

A Linux user could be forgiven for not having access to Windows, but Windows users these days have no excuse haha. xD You've got both WSL, Docker, Virtual Box and VMWare Player. All free options.

In my case, I do this on Windows.

# From PowerShell
cd cmdc
docker run -ti --rm -v ${pwd}:/workdir mottosso/maya:2020
cd /workdir

From there you could literally run what the CI is running to recreate it verbatim.

# Setup Build Environment
yum install centos-release-scl -y
yum install devtoolset-7 bc -y

# Build
export DEVKIT_LOCATION=$(pwd)/devkitBase
. scl_source enable devtoolset-7 || true
g++ --version
chmod +x ./build_linux.sh
./build_linux.sh 2020

When you exit, the environment is gone. To iterate, you could chuck all commands into a Dockerfile like this one and build it once.

cd mycontainer
echo "My Container" > Dockerfile
docker build -t mycontainer .
docker run -ti --rm -v ${pwd}/workdir mycontainer
$ # Same environment each time

Alternatively, you could keep updating that yml file for GitHub actions to run things. I expect the results to be the same across Windows and Linux, so you could keep testing on Windows and have it built on Linux.

@mottosso
Copy link
Owner

I'm not familiar with github actions so if you have any recommendations on what should be done, I'm all ear

For completeness, anything under steps is just a series of commands run in bash one-by-one, in the order they are written. There are a few keywords like ${{ matrix.maya }} in there that should hopefully make sense, but if not don't worry about it I don't think you need to use or make any of those. The sections like "Setup Test Environment" is just to make it more readable in the web UI.

@Muream
Copy link
Contributor Author

Muream commented May 26, 2021

A Linux user could be forgiven for not having access to Windows, but Windows users these days have no excuse haha. xD You've got both WSL, Docker, Virtual Box and VMWare Player. All free options.

Haha I haven't thought of that. I've been meaning to learn Docker for a while, that's a good opportunity to do that

Thanks for all the info, that'll be helpful 👍

@mottosso
Copy link
Owner

Once you pop, you can't stop. :) Docker is a game changer. That command for mayapy above can also be used to spawn any of the last releases of Maya of the past 8 years.

Try some more "tags" from here: https://github.com/mottosso/docker-maya

@Muream
Copy link
Contributor Author

Muream commented Jun 6, 2021

So I looked a bit more into this today, I'm not 100% when we should generate the stubs in the process.

I have two questions:

  1. Is it ok to only build the stubs with Maya 2022?
    pybind11-stubgen only works with python 3. If we don't expect cmdc to have any differences between Maya versions, I don't see a problem there.
    If we do, we could modify pybind11-stubgen a bit to make it work with python 2 (at first glance, that seems doable)

  2. Should the stubs generation be a separate job that's run after the build jobs?
    That would give us the flexibility of generating the stubs with only some Maya versions and not all the ones listed in the build jobs.

One note on the generated stubs:
They look mostly good but a lot of arguments end up named arg0, arg1, etc. instead of their actual name.
This comes from pybind11, not the stubs generation itself. pybind11 inserts a function signature at the top of the docstring and that signature has the misnamed arguments.
I think only the arguments with a default value are named properly.
If that issue rings a bell let me know, otherwise I'll have a deeper look later.

@mottosso
Copy link
Owner

mottosso commented Jun 6, 2021

Is it ok to only build the stubs with Maya 2022?

I should think so. Maya 2022 would have everything past versions have, so if anything 2022 is the ideal choice. Although, maybe it would also give you hints about arguments that only exists since 2022, such as MDagModifier.deleteNode(includeParents=True)?

If Python 3 is what it needs, then it is how it is. Unless it doesn't need mayapy at all?

Should the stubs generation be a separate job that's run after the build jobs?

I would have thought they could run right after the Python tests, like just another set of commands? The trouble with giving them their own job is that we then need to pass the build artifacts from one job to another along with setting up the dependency. Jobs are basically running on different machines.

How about just another block following this one?

- name: Tests
run: |
pwd
ls
mayapy --version
export PYTHONPATH=$(pwd)/build
mayapy -m nose -xv --exe ./tests

They look mostly good but a lot of arguments end up named arg0, arg1, etc. instead of their actual name.

Sounds bad, but maybe it's good enough for a first pass? This kind of thing doesn't seem to care much for backwards compatibility. Would like to merge this PR to take many small steps soon in favour of one large perfect step later.

@Muream
Copy link
Contributor Author

Muream commented Jun 6, 2021

If Python 3 is what it needs, then it is how it is. Unless it doesn't need mayapy at all?

pybind11-stubgen uses the inspect module to parse everything so it needs to be able to import cmdc. I don't think we can do that without mayapy?

I would have thought they could run right after the Python tests, like just another set of commands?

Wouldn't that make it run on every maya version in the maya-linux job?
If there's a way to conditionally run commands by checking the current {{ matrix.maya }} or something, I'm happy to do that yeah

Sounds bad, but maybe it's good enough for a first pass?

Yeah agreed. I should have something ready by next weekend I think.

@mottosso
Copy link
Owner

mottosso commented Jun 7, 2021

If there's a way to conditionally run commands by checking the current {{ matrix.maya }} or something, I'm happy to do that yeah

There is yeah, there's another step with one you can look at for reference.

Next step is looking at the GitHub Actions docs.

pybind11-stubgen uses the inspect module to parse everything so it needs to be able to import cmdc. I don't think we can do that without mayapy?

I'm not sure. :S It's perfectly capable of building cmdc without mayapy, so it seems logical that we'd be able to inspect the results without it. But what it's asking for are some of the DLLs that ship with Maya..

image

So then at least, if necessary, we could just bundle these with this repo and avoid having to call into Maya at all.

@Muream
Copy link
Contributor Author

Muream commented Jun 7, 2021

Ah, looks like I didn't write the condition to generate the stubs properly haha
@mottosso do you know if there's a good way to test the CI configuration locally instead of having to push all the time to test things?


There's one very annoying with pybind11-stubgen, it can completely freeze in some cases
So far I suspect it comes from docstrings containing an invalid signature. The solution is to just remove the signature from the docstring

Some function stubs result in something like: my_func(*args, **kwargs) -> Any

This happens when pybind11 generates a docstring with a syntax error in the signature
pybind11 gives this warning:

Generated stubs signature is degraded to `(*args, **kwargs) -> typing.Any` for
def objectColorRGB(self: cmdc.FnDagNode) -> Autodesk::Maya::OpenMaya20220000::MColor: ...
                                                     ^-- Invalid syntax

In most this is because the return (or argument) type is not declared as a binding before. Declaring it in ForwardDeclarations.inl fixes this.

There's also this issue :

Generated stubs signature is degraded to `(*args, **kwargs) -> typing.Any` for
def create(self: cmdc.FnDagNode, type: cmdc.TypeId, parent: cmdc.Object = <cmdc.Object(kInvalid)>) -> cmdc.Object: ...
                                                                          ^-- Invalid syntax

In this case parent has a default value of MObject::kNullObj. Pybind uses the value's __repr__ in the signature which isn't valid.
It's possible to override this manually by doing something like this

On a final note, specifying the arguments with py::arg("my_arg") seems to fix the issue with the arguments named arg0, arg1, etc. in the stubs.


Also, I have absolutely no idea why the tests failed on Maya 2018 there.

@mottosso
Copy link
Owner

mottosso commented Jun 8, 2021

@mottosso do you know if there's a good way to test the CI configuration locally instead of having to push all the time to test things?

Sigh, sadly no. This is typical CI. I've stopped worrying about it, just keep pushing commits. If you're feeling self-concious, I've found that you can overwrite a commit and force-push to trigger a new build without making a mark in git history.

Also, I have absolutely no idea why the tests failed on Maya 2018 there.

This, my friend, is why cmdc needs to exist. This is a crash. From not doing anything in particular. The API is riddled with these, exactly what we need to protect against, and you've just found the first one for cmdc. Congratulations! 🥳

How?

  • Absolute worst case, limit use of the function to Maya versions that won't crash. An exception is better than a fatal crash. Ideally the exception would include a message about an alternative method.
  • In the ideal case we'll be able to check the input prior to using it, making sure e.g. MObject's aren't NULL etc.

@Muream
Copy link
Contributor Author

Muream commented Jun 8, 2021

I'll have a look at the crash yeah

I'm surprised it doesn't crash in master though, I don't think I've touched the implementation of anything, unless I've messed up the merge somehow, will dig more tonight 👍

@Muream
Copy link
Contributor Author

Muream commented Jun 8, 2021

Cool so this seems to work so far, I've uploaded the stubs along with the Maya 2022 module, meaning in the release they'll only be in the linux-2022 folder.
I'm guessing that'll work but I don't really know how to test the release job


There's a lot that can be done to improve the quality of the stubs, mainly adding py::arg and py::arg_v everywhere to let pybind know of the name of all the arguments like you've started doing in MFnDagNode:

cmdc/src/MFnDagNode.inl

Lines 221 to 222 in 86cc176

}, py::arg("type"),
py::arg("parent") = MObject::kNullObj,

That's probably better suited for a second PR though so I'm happy to merge this one now


That Maya 2018 crash is also just gone and I haven't managed to reproduce it on my end

@Muream Muream marked this pull request as ready for review June 8, 2021 22:23
@mottosso
Copy link
Owner

mottosso commented Jun 9, 2021

I'm guessing that'll work but I don't really know how to test the release job

Where do they need to be? Is it also on PYTHONPATH? Simplest thing you can try is downloading the latest release and putting your stubs wherever they belong and make sure it works wherever it needs to work. I've never used stubs so I'm not too familiar with what goes into it.

Come to think of it, they should appear in the next release too so let me merge and then you can continue experimenting with the real thing. Do we want these in any way related to Maya? Like, in Maya's PYTHONPATH too? Or are they completely separate from the cmdc .pyds? :S The canonical way would be to put them under..

~/maya
  /modules
    /cmdc
      /scripts
        /stubs-here.stub

But that would only make sense if Maya is consuming them. Anyway, I'll leave this to you!

@mottosso mottosso merged commit 0548492 into mottosso:master Jun 9, 2021
@mottosso
Copy link
Owner

mottosso commented Jun 9, 2021

Looks like they made it. https://github.com/mottosso/cmdc/releases/tag/v0.1.4

One option is simply copying the single pyi file to the other folders at release time, like I'm copying the .mod file currently, just before zipping them up for final upload.

- name: Create distribution
run: |
cp ./cmdc.mod modules/
zip -r cmdc-0.1.0.zip modules/

@Muream
Copy link
Contributor Author

Muream commented Jun 9, 2021

The stubs are never really useful in maya, they're meant for your editor to give you nice autocompletion, typing information, etc.
In vscode you'd add the folder where the stubs live to python.analysis.extraPaths

Once generated the stubs don't need the .pyd files at all, I think some libraries even just publish their stubs independently on pypi.

In our case, I feel like something along those lines would make sense

~/maya
  /modules
    /cmdc
      /stubs
        /cmdc.pyi

I'll start another PR that tackles all the ugly arg0 stuff soon and will change the location of the stub there as well. 👍

@mottosso
Copy link
Owner

mottosso commented Jun 9, 2021

Sounds good!

@Muream Muream mentioned this pull request Jun 9, 2021
@Muream Muream deleted the generate_type_stubs branch June 29, 2021 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants