Origen-SDK / o2

MIT License
4 stars 0 forks source link

[OM] Infastructure and Python Diff API #143

Closed ginty closed 2 years ago

ginty commented 2 years ago

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:

bail!("Some error")  // Return immediately with an error

error!("My error was '{}'", some_var)     // Generate an error instance

some_func().context("While we were doing...")?;   // Decorate upstream error messages

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:

>>> from origen_metal.utils.differ import has_diffs
>>> has_diffs("filea.txt", "fileb.txt")
Traceback (most recent call last):
  File "<console>", line 1, in <module>
RuntimeError: No such file or directory (os error 2)
 - When opening 'filea.txt'
>>>

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:

image

And we can run pdoc on the Python stubs to generate nice API documentation:

image

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.

coreyeng commented 2 years ago

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 sphinx extension setup than that, plus a bunch of docs that need conversion. But, like I said, I'm not a massive fan of Sphinx myself. I didn't think it was that bad, but wasn't as intuitive as what we had for O1.

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.

coreyeng commented 2 years ago

Does the new error play nicely with what's in Origen? Looks like it should. Can we look at moving the entire Error and Result from Origen into Metal and using it there?

ginty commented 2 years ago

Hi @coreyeng, thanks for the quick review! Yeah I was expecting to discuss the docs stuff at the next meeting, next week I think.

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. Feel free to go ahead and do that if you want, I did for now simply because I didn't want to add all those deps only for that and when I wasn't sure if Origen would use it or not.

Good news re. Py 3.7.

coreyeng commented 2 years ago

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.