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

Make python client available in binary wheels, and change its name to magika-python-client #930

Open
reyammer opened this issue Jan 27, 2025 · 3 comments
Labels
python Pull requests that update Python package

Comments

@reyammer
Copy link
Collaborator

This is the current situation:

  • binary wheel: magika => rust client. There is not python client.
  • pure python wheel: magika => python client. There is no rust client.

This is not ideal:

  • if for any reason pip selects the pure python instead of the rust one, clients will inadvertely use the python one, which has somewhat the same features, but is slower and its output is not exactly the same
  • it is handy to have the python client in the binary wheel as well, as I often use it for quick tests (and, for the python client is trivial to support v1 and other versions; I could keep support for old versions around and use to compute some metrics. We would of course only ship in the package the most recent model, but the python client can easily be pointed to any model in the assets folder).

I propose to do the following:

  • binary wheel: magika => rust client. magika-python-client => python client.
  • pure python wheel: magika => print a warning that this is a pure python wheel with no rust, and that one should either install the proper wheel, or one can fall back to using the python client, with magika-python-client. magika-python-client => python client

@ia0, thoughts?

@reyammer reyammer added the python Pull requests that update Python package label Jan 27, 2025
@ia0
Copy link
Member

ia0 commented Jan 27, 2025

This looks good to me. But then I'm thinking, if we're fine having a separate wheel for the python client, what's the disadvantage of having a separate wheel for the Rust client as well? At least we would have the benefit of not having to hack the version numbers. That's small now that it's done, but just wondering what's the disadvantage.

@reyammer
Copy link
Collaborator Author

Good points.

The way I see is that we should do whatever we can to hide complexity from the users. For example, I think having a separate wheel for the python client is totally OK because most users will not have any idea about it (with the tradeoff that we are adding complexity on our side; which I think it's preferrable). And it's big win to be able to say to users "just do pip install magika" without needing to explain various if/else scenarios.

Now, about this proposal... I tried, and it did not really work that well.

No problems for the pure python wheel: 1) "magika-python-client", and 2) we now have "magika" (which shows a warning about missing rust, points user to open a github issue, and mentions the python fallback "magika-python-client". This is done: #931.

The problem is about the binary wheel: maturin is very unhappy about me specifying additional entry points (called "scripts" in the context of pyproject.toml):

  × Failed to build `magika @ file:///workspace/magika-github/python`
  ├─▶ The build backend returned an error
  ╰─▶ Call to `maturin.build_editable` failed (exit status: 1)

      [stdout]
      Running `maturin pep517 build-wheel -i
      /home/ml_user/.cache/uv/builds-v0/.tmpRRC2sW/bin/python --compatibility off
      --editable`

      [stderr]
      🍹 Building a mixed python/rust project
      🔗 Found bin bindings
      📡 Using build options locked, bindings from pyproject.toml
          Finished `release` profile [optimized] target(s) in 0.08s
      💥 maturin failed
        Caused by: Defining scripts and working with a binary doesn't mix well
      Error: command ['maturin', 'pep517', 'build-wheel', '-i',
      '/home/ml_user/.cache/uv/builds-v0/.tmpRRC2sW/bin/python', '--compatibility',
      'off', '--editable'] returned non-zero exit status 1

      hint: This usually indicates a problem with the package or the build environment.

Related maturin's issue: PyO3/maturin#368

So, for now I've only modified the pure wheel; I'll leave the binary wheel untouched for now, which means no python client for those. I think that's fine; my main concern was to not have people assuming they are running the rust binary while in fact they are running the old client (which could at some point be out of sync in terms of features).

@ia0
Copy link
Member

ia0 commented Jan 29, 2025

Sounds good to me. The Maturin stuff is a bummer, but I agree not that bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests that update Python package
Projects
None yet
Development

No branches or pull requests

2 participants