-
Notifications
You must be signed in to change notification settings - Fork 0
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
[OM] Infastructure and Python Diff API #143
Conversation
Looks good! I do think we should talk about the doc stuff in the next meeting. I don't think metal and the app should use different internal docs systems so if we're moving away from Sphinx for sure, I'd like to know. I don't have a ton written anyway so easier to just eat the Does Pdoc have theming? Specifically, a dark theme? I've become too used to dark themes. I do actually like the theme we had in the existing O2 docs. It was a huge pain to setup but I thought the end result looked good. I as actually about to say the same for Python 3.6! I did update to PyO3 0.14 on this branch (which should help you in pull more stuff if you need as you don't need to translate from 0.11.1 to 0.14.3), but Python 3.6 gave some really nasty stack errors and required going back adding this unsafe block to adequate get things to work as before. I agree though - based on (this)[https://www.python.org/dev/peps/pep-0494/], looks like 3.6 will be deprecated end of this year and I'd rather start looking at support 3.9 and 3.10 than back-support 3.6. |
} | ||
} | ||
|
||
// This needs re-worked for current PYO3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this on a PyO3 branch. I've found it to be pretty helpful. Can revisit after that merge.
Does the new |
Hi @coreyeng, thanks for the quick review! The new error will play fine with Origen once the commented out conversions are all added, should be a drop in replacement at that point. Good news re. Py 3.7. |
Sounds good. We can talk about all the doc stuff next week then. I think merge if you're ready then I will look into a PR for Py03 0.14 on the app side. I think you're using 0.14.2, but based on this, I think metal should bump to 0.14.3, just in case. I can handle that though. Should be easy. |
This PR adds the first Python API (for file diffing) and some basic infrastructure to allow OM to appear like a conventional Python package as far as doc generation and IDEs like VSCode are concerned.
I removed the use of the
anyhow
library for error handling as I ran into the forgotten foe of not being able to use?
in pyapi functions since it's error does not convert to a PYO3 error.So I've added an OM version of Origen's Error, Origen can keep its own or eventually the OM one can evolve to make it a drop-in replacement.
I did like the
anyhow
features though, so I've added a poor man's version here which seems to do everything I've needed from it anyway:The last one is most useful and it just appends the context message onto the error, though we can evolve this as necessary in future if it needs to be fancier (e.g. store a stack of messages in the error and then provide different ways to format them).
Here's an example of context having been added when opening a missing file:
A self-explanatory Py API has been added to expose the differ to Python, I added tests for it and all working (I don't expect to replicate all Rust tests in Python like this in future, but as this is the first one I just made sure).
I then played around with how to make this appear like a regular (fully annotated/understood) function as far as something like VSCode is concerned and I came to the conclusion that PYO3 doesn't really support that currently.
I saw some tickets about generating Py stubs but no sign of much activity on it.
So the system I came up with here is to fully define the Python API structure in the Python code, but just populate it with type-hinted stubs.
These are directly analagous to a header file in C terms, with the pyapi code being the C source/implementation files.
I also added a
py_submodule
helper to fully register the code into Python (gleaned from some PYO3 ticket comments) and everything works nicely.When working in VSCode it feels just like working with a regular Python package now:
And we can run pdoc on the Python stubs to generate nice API documentation:
Just one minor quirk is that the
from _.origen_metal import *
from the top-level Python file needs to be commented out or PDoc gets confused.Another thing is that PDoc required a minimum of Python 3.7.
I haven't left the PDoc dependency in because of that, but wondering what the thoughts are with making OM min Python 3.7?
It feels like the Py community might be moving on and we should keep pace with that, and within AMD we don't need 3.6 support anyway.