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

Add python stub files #493

Closed
wants to merge 42 commits into from
Closed

Add python stub files #493

wants to merge 42 commits into from

Conversation

nworb-cire
Copy link
Contributor

IDEs have a very hard time introspecting cereal structures, which can hinder development. I used this tool to create python stubs from the capnp files. The tool seems to be a little bit broken, and a few manual corrections were necessary, so I do not include the generator script here. Unless that tool is fixed in the future, I suggest that additional fields be added to the stub files directly, rather than being autogenerated.

806907cd33ef2647|2023-07-04--17-58-54--0 is a route of master + these changes. I noticed no odd behavior while driving the car, but have not reviewed logs to see if there was any unwanted behavior. If any overhead was added from these changes, I hope it is negligible.

Note: legacy.NavUpdate.Segment.from is not a valid python identifier, so I commented it out from the stub file.

@nworb-cire
Copy link
Contributor Author

Forgot to mention this resolves #212.

mypy seems to be complaining about the stub files having the same names as the non-stubs; I'm unsure how to make it happy and achieve the goal we're looking for, except to exclude one or the other in a mypy config.

@nworb-cire
Copy link
Contributor Author

I ended up forking the capnp stub generator tool and making the required alterations to get it to work. Those changes are:

  • Suppression of a dataclass post-init sanity check (perhaps ought to investigate more, though everything seems to be working fine)
  • Auto-commenting lines in the .pyi files that have invalid python identifiers (for now these exceptions are hardcoded)
  • Fixing top-level enums not being included in the .py output
  • Silencing something having to do with capnp import errors (ought to investigate and fix, though again everything does seem to work fine)
  • Linter fixes

I have no desire to try to upstream these changes to the original repo, especially considering that it seems dormant.

I also added a generate_stubs.sh script that handles the script generation. It appears to order everything deterministically, so there is no diff when running it over and over again. This script should probably be put in an automated workflow somewhere, although I am unsure where the best place to put that would be.

Please let me know if there are any necessary further changes to get this merged-- it is very helpful to development and I'd like to see it in as soon as possible.

@adeebshihadeh
Copy link
Contributor

adeebshihadeh commented Sep 20, 2023

Thanks for the PR! This has sat around for a while since it's a +12k diff and adds overhead to otherwise simple cereal changes. Instead of checking in the stub files, can we just generate them at compile time (plus gitignore)?

@nworb-cire
Copy link
Contributor Author

Down to <3600, but I have an idea to remove the canpnp-stub-generator dependency in non-dev environments. That should make the diff quite minimal.

@adeebshihadeh
Copy link
Contributor

Looks like it's just a normal Python package; can we just install it with pip?

@nworb-cire
Copy link
Contributor Author

We can clone it from their gitlab and do a local pip install (it's not on pypi), but the issue is that there are a small number of nontrivial changes needed to either fix some bugs (their package might be targeted towards a different capnproto version?) or format the output how we would like.

My idea was to clone and apply a patch before installing with pip, though I'm open to other suggestions.

@adeebshihadeh
Copy link
Contributor

It doesn't need to be on PyPI. pip allows you to install from a git repo if it's setup right. How about I fork the repo to the commaai org, you make a PR with the changes, then we pip install that version?

@nworb-cire
Copy link
Contributor Author

That's a good compromise, and cleaner than what I was thinking. Comment back here once you've done so and I'll be right on it.

@adeebshihadeh
Copy link
Contributor

@nworb-cire
Copy link
Contributor Author

I forgot that the .py files (as well as .pyi) are now autogenerated from the capnp schema, so we do still have to deal with that in some way. I think auto-updating them with a workflow may still make some sense.

@nworb-cire
Copy link
Contributor Author

Should be good to go once commaai/capnp-stub-generator#1 is merged; for now the aforementioned workflow will fail.

@adeebshihadeh
Copy link
Contributor

Why are generated files still being checked in? They should just be part of the build system.

@nworb-cire
Copy link
Contributor Author

If they are to be built in all cases, the pip install etc. will need to be done on-device as well. I was trying to avoid the need to install any packages outside of a dev environment.

@adeebshihadeh
Copy link
Contributor

Thanks for the PR, @jnewb1 is working on this in #546

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.

2 participants