Replies: 2 comments
-
Thank you for leading the effort on simplifying this. |
Beta Was this translation helpful? Give feedback.
-
I'm all for using Maturin as well, but I have not been able to get it to work with the Origen package. I do know Maturin is still being developed but whenever I've tried to include a build script, which we needed to load the binary, or support other options available within Poetry, it would never work. Although we deviate from the PyO3 convention by not using Maturin, we deviate from the Poetry convention by using it. It is much simpler if we can get it to work. I will need to revisit the release probably early next month anyway. I can start with Maturin, but if it still cannot work as I need it to, I'll need to revert it. Though, I can look at dual-support too since I don't think anything Maturin needs actually conflicts with what I need. Possibly we have OM built with Maturin available so if the build script breaks, you guys aren't stuck. That fourth bullet too is something I noticed but was also never able to get a good solution for. An intermediary could be just simple script that we run during the action that check the dependencies match and fails the build accordingly. Or, we can make in fancier like automatically keeping in sync. That's also reminiscent of something I don't about Poetry, wherein it needs bounded Python versions. So, updating a new supported Python version is actually kind of a pain, so much so that I made a custom command for it. |
Beta Was this translation helpful? Give feedback.
-
@coreyeng, @priyavadan, @rlaj, this is to document the current status of the Python package release flow for whoever needs it next...
Following the discussion here I have been able to release Origen metal (0.4.1.dev4) for Linux only after some wrestling with the Publish Packages action.
I've raised this point before, but I think the approach of not using Maturin to create the Python package is wrong and goes against PYO3 conventions.
Specifically, the approach of running the release action inside the many linux container doesn't work since the latest github actions do not run on it. I almost got it to work by dialing back the action versions to one that would work, but I ran into a roadblock at the end since it needs
actions/upload-artifact@v1
to run in the container, but this version is no longer compatible with the latest Github workspace layout.I think that is pretty typical of what we could expect in the future even if it had been possible to get a tool setup that worked in the short term.
Therefore I decided to change the OM build flow back to using Maturin since it has been working fine for us in AMD for similar builds.
This simplified the action flow considerably, however I did have to make the following changes on our side:
pyapi_metal
crate toorigen-metal
pyproject.toml
into therust/pyapi_metal
dir (this proved to be important to get the extension correctly namedorigen_metal._origen_metal
)0.4.1.dev4
to0.4.1-dev4
[project]
section inpyproject.toml
and this has to be manually kept in sync with the deps in[tool.poetry.dependencies]
. This is the biggest downside going forward, with further effort it may be able to eliminate this, but I didn't pursue it for now and I've marked up the file to jog our memories in the future.[build-system]
entry topyproject.toml
, it can't be there by default since it killed the Windows flow, however once the Windows flow is also on Maturin this should go awaypoetry_build.py
script since it was killing the regression test following these updates and I don't think it is required anymoreThe current Windows flow did build a wheel without any intervention, however it is rejected by pypi.org, I think because something is wrong with the Metadata, but I didn't debug much.
As I don't need Windows for now I just commented it out, but ultimately I think it should also just convert to Maturin and let others deal with such details, here is an example of a Maturin windows build - https://github.com/messense/py-dissimilar/blob/d1d813ad287aab81a3730777021d61321503ce01/.github/workflows/CI.yml#L46
I have also commented out the Origen package flow as it will have the same problems as above and can likely be fixed in the same way, however I haven't made any changes to it yet.
Internally within AMD I have had no issues with including additional files, such as the
origen
binary, using Maturin.Beta Was this translation helpful? Give feedback.
All reactions