-
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
Metal Integration Waypoint #1 #145
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…l. Update pyreadline to more modern pyreadline3. Try Python 3.9 and 3.10.
ginty
requested changes
Aug 31, 2021
ginty
approved these changes
Aug 31, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello,
This is a first attempt at integrating
metal
as a dependency fororigen
. To start, I just did some very basic things to get my bearings and not bloat the PR.Origen Metal Dependency and More Version Verbosity
I added version info to pyapi_metal and origen_metal. This is very minor, but as we have more moving parts, I think just having a record may be helpful while potentially allowing for version checks in a release script to ensure everything went alright. When run with
debug
verbosity,origen -v
will spit out theorigen_metal
(Python),pyapi_metal
, andorigen_metal
(Rust) versions:I added
pyapi_metal
as a dependency topyapi
, allowing utilities to be used in both places. As this is meant only for non-pyclass/pymodule/etc. usage, this shouldn't cause library-size bloat nor duplicate definitions. All python stuff should be used fromorigen_metal._origen_metal
directly and not be reproduced in_origen
.I didn't see that
pyo3
supportspathlib.Path
objects directly yet, so I just moved over the pypath macro I had as a very basic test case.Interactive Startup
I moved the interactive setup into
origen metal
, made it a bit more generic, and removed it fromorigen
. This is the same setup as before which just allows console history to function in Windows.Python 3.9 Support
When I moved the interactive stuff, I also updated to pyreadline3, which is a forked, active version of pyreadline that was blocking 3.9 support. Fixing this, 3.9 just worked. I updated the regression tests and release scripts to include 3.9. Haven't run the release scripts and am not 100% on this, but based on the python-setup action, this is my best guess.
Python 3.10 Lookahead
Python 3.10 works alright in my environment with some prerelease Poetry version, but I could not for the life of me get the regression script to pass. In my environment, all is fine, but I can't reproduce the error. Python 3.10 is still unofficial, but looks like its targeting October to be official. I'll continue tinkering with this to see if we can get some lookahead. At the very least we'll need a Poetry update, but hopefully 1.2 will be out of prerelease by then. However, in debugging this, I did make two changes:
MINIMUM_POETRY_VERSION
.PyO3 0.14.4
PyO3 0.14.4 was released. Seems pretty minor, but I bumped up the version nonetheless.
Minor Test Updates
A minor problem did come up with the regressions in the example app. When
origen_metal
was added as a local dependency, thetests
directory there, which has an__init__
and is therefore a module, superceeds the one in the test app. So, an__init__
was added there to make it a module, but this caused some of the tests to fail as the module path for these is nowtests
instead of empty. These were minor and expected given the needed update, but just pointing them out regardless.SSL Requirement
In origen, I removed the SSL requirement from Windows builds since it requires a separate install for Windows and wasn't being used. I did the same for metal but I can look at getting this to work again. I guess this built fine on the Windows machines during the regressions, I should probably give this some more thought in the future.
Side Note
I plan to keep this branch around to work on generalizing and cleaning up things that could go in
metal
. I plan to do more PRs off of this branch as this occurs. If anything is known to be needed sooner, let me know and I can likely prioritize those items.Thanks!