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

Initial Version #1

Merged
merged 107 commits into from
Mar 4, 2024
Merged

Initial Version #1

merged 107 commits into from
Mar 4, 2024

Conversation

mbway
Copy link
Collaborator

@mbway mbway commented Feb 29, 2024

This builds off of the original PR in the maturin repo: PyO3/maturin#1748

I have finished enough of the import hook that I think it’s ready for review. There are still improvements to be made (some ideas below) but it’s mostly just finishing touches. I am likely to be unavailable for at least the next week but I suspect reviewing will take a while anyway.

Some general points/questions about the project and repo:

  • will I be able to review PRs in this project or am I just a regular contributor?
  • can you open discussions on the repo please
  • I haven’t put anything in place for releases to pypi or automatic update PRs
  • what should be done about hosting documentation? Should the documentation be hosted as part of the maturin documentation or elsewhere?

here is a CI run where all the tests passed: run

At the moment I think the basics are solid enough to release but things aren't perfect. Some things still on my todo list:

  • importlib.reload support on Windows
    • I developed the feature on Linux and windows has different requirements since you cannot modify a file if another process has opened the file.
    • might make sense to re-work the way the package reloading is implemented so the behaviour of the file and project importers are the same
  • create an interface called PackageBuilder or similar, for users to customize how maturin is called instead of hard-coding
    • the user can specify the flags passed to maturin on a per-package basis but full control over the call including environment variables and pre- or post-processing would be better
    • for the rust file importer, an interface could be provided for exactly how the user wants to set-up the generated project for the rs file
      • could provide a rustimport compatible loader that allows dependencies to be specified in comments in the rs file, or a loader that allows you to specify dependencies in a toml file adjacent to the rs file
    • Currently the user can only specify a MaturinSettings object used for building all packages
  • improve tests
    • parallelise the test runner (create multiple venvs and multiplex over them)
    • try methods for speeding up tests, eg cranelift backend, uv for installing packages
    • code coverage
    • profiling (breaking down into time spent compiling, installing, running python code)
    • refactor tests to improve consistency
  • test more edge cases
    • eg renaming project or other fundamental settings between builds
    • interaction between importlib.reload and subprocesses/threads

mbway and others added 30 commits January 21, 2024 15:13
when running tests in parallel on a weak machine the concurrent test
worker subprocesses will be waiting on the first compilation to finish.
the other tests should not have any contested locks so the extra timeout
is only important for the concurrent tests.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know maturin uses https://pre-commit.ci/ and you can switch to that if you prefer, but this works fine too IMO. takes ~1min to run

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if maturin uses an automated tool for managing the changelog. I haven't set anything up in this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changelog isn't automated in maturin ATM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my idea is to use maturin from pypi during testing, but get the test crates from a submodule pinned at an appropriate version. The package_resolver rust project is also required for testing and needs access to the maturin source code so this submodule serves both purposes

@messense
Copy link
Member

messense commented Mar 1, 2024

  • will I be able to review PRs in this project or am I just a regular contributor?

I've sent you an invitation to colab on this repo.

  • can you open discussions on the repo please

Done.

  • I haven’t put anything in place for releases to pypi or automatic update PRs

We can do that later.

  • what should be done about hosting documentation? Should the documentation be hosted as part of the maturin documentation or elsewhere?

Part of maturin should be fine.

Copy link
Member

@messense messense left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, let's merge it and iterate on it.

@messense messense merged commit b6435ba into PyO3:main Mar 4, 2024
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