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

pixi run pip install is overridden by subsequent call to pixi #1233

Open
2 tasks done
abey79 opened this issue Apr 19, 2024 · 28 comments
Open
2 tasks done

pixi run pip install is overridden by subsequent call to pixi #1233

abey79 opened this issue Apr 19, 2024 · 28 comments
Assignees
Labels
🐞 bug Something isn't working 🐍 pypi Issue related to PyPI dependencies

Comments

@abey79
Copy link

abey79 commented Apr 19, 2024

Checks

  • I have checked that this issue has not already been reported.

  • I have confirmed this bug exists on the latest version of pixi, using pixi --version.

Reproducible example

We encountered this issue while working on rerun-io/rerun#5966 (can be reproduced with commit 44412db2519cf2a37b0bd9d951cc4d8630eac037).

rm -rf .pixi
pixi run -e examples clock
# error indicating that rerun-sdk is pulled from pypi as expected

pixi run -e examples py-build  # calls maturin develop, which calls pip — should fix the error

pixi run -e examples clock

-> leads to the same initial error, as if py-build had no effect.

Issue description

It actually turns out that py-build does install a build-from-source rerun-sdk correctly, but this is reverted by the last call to pixi, possibly due to #998?

Interestingly, activating a shell is a possibly work-around:

rm -rf .pixi
pixi run -e examples clock
# error indicating that rerun-sdk is pulled from pypi as expected

pixi shell -e examples
pixi run -e examples py-build

clock  # -> works as expected

Another case where it appears to work as expected is in CI, with an environment set for the setup-pixi action: https://github.com/rerun-io/rerun/blob/36f00a5b0bd3edf06bc0f4ec65ce58f86a32e458/.github/workflows/reusable_build_examples.yml#L66-L86

Expected behavior

Ideally, pixi would make it easy and robust to manually "override" packages in the environments, including via pip specifically for when there is no other way (e.g maturin). This would cover at least two workflow:

  • dev environment (run ${STUFF} against a build-from-source rerun)
  • CI (run/test ${STUFF} against a CI-built wheel)

Workaround

For our particular case, @jleibs just had a workaround idea that appears to work:

  1. we created a "empty" rerun-sdk local package somewhere (that would install a differently named empty python file)
  2. we added an explicitly dependency on it in pixi.toml for the examples environment

That way, examples defaults to failing (no module named 'rerun'), and somehow a subsequent pixi run -e examples py-build doesn't end up being overridden.

pyproject.toml:

[build-system]
requires = ["hatchling"]
build-backend = "hatchling.build"

[project]
name = "rerun-sdk"
version = "0.10.0"

[tool.hatch.build.targets.wheel]
packages = ["empty_rerun_sdk.py"]
@abey79 abey79 added the 🐞 bug Something isn't working label Apr 19, 2024
abey79 added a commit to rerun-io/rerun that referenced this issue Apr 19, 2024
@tdejager
Copy link
Contributor

I think I understand the use-case, it begs the question though, is the package that py-buid creates also locked by pixi itself?

@abey79
Copy link
Author

abey79 commented Apr 19, 2024

I should have clarified: py-build runs maturin build, which creates our rerun-sdk package. This is what all our examples depend on. So without the explicit dependency on the "empty" rerun-sdk package is [pypi-dependencies], pixi locks on the pypi version of it.

@tdejager
Copy link
Contributor

So the use-case is:

  • You want to use the latest rerun-sdk from source, and have the examples depend on rerun-sdk as well?

Couldn't you add rerun-sdk as an editable with a pyproject.toml with the maturin backend?

@abey79
Copy link
Author

abey79 commented Apr 19, 2024

We'd rather keep a "lax" dependency on rerun-sdk on our example to cover the end user use case of running examples from the latest branch (corresponding to the last release). In that case, pulling from pypi is the desired behaviour, and avoid any compilation (or rustc to be installed at all—most of our users are not rust devs).

Edit: also it wouldn't cover the CI use case, where we want to run against a very specific, pre-built wheel.

@tdejager
Copy link
Contributor

tdejager commented Apr 19, 2024

Couldn't you just make a seperate environment for running with the latest version?

EDIT:

Also is my read of your use-case correct ):?

@abey79
Copy link
Author

abey79 commented Apr 19, 2024

Four our use-case (use-cases, really), our ideal requirements are:

A) As a user:

  • I can run any example from the latest branch against a pypi version of rerun-sdk
  • In particular, no compilation should be needed (rust not installed).
  • I can do that with standard python tooling (pip install ...).
  • I can do that with any python version I might have around (provided that it's compatible with rerun-sdk and the particular example at hand)

B) As a dev:

  • I can run any example in editable mode.
  • Running an example should not automatically trigger any rust compilation (most rust changes do not impact the python sdk) to ensure fast iteration cycles (eg when working on said example).
  • I can manually trigger a SDK install from source (aka py-build) when needed. (It's ok to have to do that at least once after initial environment creation.)
  • It's ok to "force" dev to use pixi.

C) As CI:

  • I can run any example against a CI-built rerun-sdk wheel (see this in action in OP's link).
  • I can run any (compatible) example against any supported Python version (and, again, against a specific wheel).
  • Stuff is cached as much as possible such as to reduce CI $$$

(rerun-sdk currently support 3.8-3.12, some examples have further restrictions)

@abey79
Copy link
Author

abey79 commented Apr 19, 2024

I believe (A) forces examples to have a lax dependency on rerun-sdk

@tdejager
Copy link
Contributor

Thanks, let me see..

So for a), I think this is already possible right:

  • The user clones the repo.
  • Navigates to the example, uses any package manager that does not require specif tool sections pdm, pip, uv etc. to install.
  • Runs it and uses the rerun-sdk from pypi.

B) I believe we should support

  • Do you mean rust compilation for the rerun-sdk or the maturin wheel build? For the wheel build, for editables we cache the pyproject.toml similar as to what maturin does I believe. We should only trigger a rebuild when the metadata changes, at least in edtable mode. Installation should be fast for edita
  • I do see this happening a lot more than it's supposed to, but those we should fix on our end.
  • If that is working you could add an editable dependency to rerun-sdk that would trigger a rebuild when it should. I believe that should cover B?

C) You are right there is no real good support for it as long as the rerun-sdk is managed in the lock as well.

@abey79
Copy link
Author

abey79 commented Apr 19, 2024

I think your summary matches our experience. The workaround I mentioned in OP provides us with an escape hatch for (C), and, as a dev, for the odd time you might still need to install an alternative rerun-sdk (e.g. some random wheels from CI for testing purposes, or a quick test against the last release from pypi, etc.).

(Thinking of it, I should have listed (B-extra) which is the ability as a dev to occasionally install any rerun-sdk from anywhere because.... life happens 😄)

@tdejager
Copy link
Contributor

Yeah for B-extra, you would need the ability to override the locked dependency in some way. Maybe checking the INSTALLER would help "break" the environment. However, the question is then, how to "un-break" it :)

@tdejager
Copy link
Contributor

tdejager commented Apr 19, 2024

I think, and that's kinda totally unrelated to your issue, but somehow it does come back to it. The reason we need to install the environments when using pypi-dependencies is the fact that we need python to be able to resolve python dependencies because of sdists. Because of this we cannot use the --no-install (that skips installation) flag, as we need to install.

Two ways of going about this:

  • Be able to define that you only want wheels in a environment (not useful for you) as an editable is not a wheel. But potentially useful in CI if the wheel is built seperately or in a sperate env at least. That way you could record it as a direct-url dependency.
  • Have a sdist build environment specification in pixi. This would be a shared environment that is just used for building wheels. We only need to resolve and install this once, or not a lot of times at least.

If we have that we could reinstate the full functionality of --no-install (if there is a sdist build env), and you could go willy nilly on your environment and "unbreak" it with a pixi install.

@tdejager
Copy link
Contributor

@abey79 we did some improvements to this workflow in the mean-time, that were discussed with you guys as well outside of this issue.

Is this enough to close this one for now :)?

@jleibs
Copy link

jleibs commented Sep 9, 2024

The exact description of the issue is somewhat incorrect, but we are still frequently struggling with behaviors related to this workflow.

Things have evolved such that our pixi.toml currently includes:

[feature.python-dev.pypi-dependencies]
# Install the `rerun_py` as a package in editable mode.
# This is very similar to `pixi run py-build`, and dispatches to maturin by way of PEP621.
# However, pixi doesn't know how to track the rust dependencies of the python package, so
# you still need to `pixi run py-build` in the correct environment if you change the rust code.
rerun-sdk = { path = "rerun_py", editable = true }

[feature.wheel-build.tasks]
# Build and install a development version of the rerun-sdk Python package.
#
# This only needs to be called when you have made changes that would impact the rust bindings of
# the python package. The python code will be imported directly from the source folder and will
# reflect changes immediately upon re-import.
# Note:
# - pip (which is called by maturin develop) doesn't recognize conda/pixi envs as venv, and thus complains if
#   configured to not install outside venv (which is a good practice). PIP_REQUIRE_VIRTUALENV=0 disables this check.
# - RERUN_ALLOW_MISSING_BIN is needed to allow maturin to run without the `rerun` binary being part of the rerun-sdk
#   package.
py-build-common = { cmd = "PIP_REQUIRE_VIRTUALENV=0 RERUN_ALLOW_MISSING_BIN=1 maturin develop --manifest-path rerun_py/Cargo.toml --extras=tests", depends_on = [
  "rerun-build", # We need to build rerun-cli since it is bundled in the python package.
] }

[feature.python-tasks.tasks]

# Dedicated alias for building the python bindings for the `py` environment.
py-build = "pixi run -e py py-build-common"

This was working for a while and behaving in a way that we were mostly happy with where Pixi WOULDN'T re-build our rust lib every time we created a pixi environment. However, that seems to have changed after a recent update.

Now any time we create the pixi environment, pixi is dispatching to maturin and re-building our rust library. This is sometimes nice, in that it picks up the latest rust changes, but also super annoying if you wanted to build those bindings manually with user-specified flags, etc. and then that library version gets replaced as soon as you run pixi again.

@ruben-arts
Copy link
Contributor

@tdejager Do you know if this makes sense?

@ruben-arts ruben-arts added the 🐍 pypi Issue related to PyPI dependencies label Sep 18, 2024
@tdejager
Copy link
Contributor

Yeah, I read this but did not reply sorry. I would have to see/play with it to see what happens, I do know that mesonpy rebuilds on editable installs as well see: https://meson-python.readthedocs.io/en/latest/how-to-guides/editable-installs.html so what I'm reading here seems to be in line with that.

It seems there is a workflow issue we need to figure out here though.

@jleibs
Copy link

jleibs commented Sep 19, 2024

FYI, I've not reproduced the issue myself, but I've also started hearing reports of pixi now caching (and worse, using the cache to restore) the contents of the librerun_bindings.so which is a totally workflow-breaking thing.

One user (on OS X if it's relevant -- need to find what version they're on) basically now always gets the bindings from the first version of the editable build that pixi invokes.

@tdejager
Copy link
Contributor

tdejager commented Oct 1, 2024

Yeah so that last thing sounds like a bug indeed, if we can find a way to reproduce it would be great. Are you guys using the maturin_import_hook, I didn't see it throughout the code, but maybe I missed it?

@tdejager
Copy link
Contributor

tdejager commented Oct 1, 2024

This was working for a while and behaving in a way that we were mostly happy with where Pixi WOULDN'T re-build our rust lib every time we created a pixi environment. However, that seems to have changed after a recent update.

Now any time we create the pixi environment, pixi is dispatching to maturin and re-building our rust library. This is sometimes nice, in that it picks up the latest rust changes, but also super annoying if you wanted to build those bindings manually with user-specified flags, etc. and then that library version gets replaced as soon as you run pixi again.

What do you mean with create just re-install an existing one or create from like an empty installation?

@tdejager
Copy link
Contributor

tdejager commented Oct 1, 2024

Hi @jleibs, ignore my previous comments :) so I've been testing against the rerun repository a bit more this morning. Maybe you could give the exact reproducer of the situation you find annoying. What I tried, and my observations:

  1. clone rerun repo.
  2. Run pixi install
  3. Run pixi r py-build-common. This seems to build everything.
  4. Run pixi install -e py. This seems to take long the first time creating this environment, I suppose it uses a maturin build like command for the editable python dependency.

However, changing the rust code or python code afterwards and running pixi install -e py does not seem to trigger any rebuilding. I think code changes to the python code should be reflected though, because it is an editable python project.

Is step 4) the one you deem to be an annoyance to the workflow? If so, the follow-up question would be: because you are using the maturin build backend, isn't a rust compilation something you expect when building the wheel? Additionally, when I remove that environment rm -rf .pixi/envs/py and did not change any rust code, it seems to be using the cached wheel, which seems.. okay.. to me I think!

Would you be able to re-frame the issue in the behavior you would like to see for which commands?

@jleibs
Copy link

jleibs commented Oct 1, 2024

Ok, using:

commit 45add8d9cb7c2384062f3b50df24df2f8d385a63 (HEAD -> main, origin/main)
pixi 0.30.0

Here is the major issue:

First verify things work. This possibly forces forces an initial build of the wheel.

➜  pixi run -e py python -c 'import rerun as rr; rr.init("rerun_example_pybuild")'
DEV ENVIRONMENT DETECTED! Re-importing rerun from: /home/jleibs/rerun/rerun_py/rerun_sdk

(The dev environment stuff is expected; I don't think this is breaking anything but worth keeping in mind).

Now, edit rerun_py/src/python_bridge.rs and insert a log statement to the new_recording method around line 201, e.g.:

fn new_recording(
    py: Python<'_>,
    application_id: String,
    recording_id: Option<String>,
    make_default: bool,
    make_thread_default: bool,
    application_path: Option<PathBuf>,
    default_enabled: bool,
) -> PyResult<PyRecordingStream> {

    re_log::info!("Edited bridge");

    // The sentinel file we use to identify the official examples directory.
    ...

Now re-build the package. This should modify the .so, so we'll confirm that fact as well.

➜  md5sum ./.pixi/envs/py/lib/python3.11/site-packages/rerun_bindings/rerun_bindings.abi3.so
7ca8aa7c0e7a00c8665e7f5be4a828fd  ./.pixi/envs/py/lib/python3.11/site-packages/rerun_bindings/rerun_bindings.abi3.so

➜  pixi run py-build
...
📦 Built wheel for abi3 Python ≥ 3.8 to /tmp/.tmpRCuk8C/rerun_sdk-0.19.0a1+dev-cp38-abi3-linux_x86_64.whl
✏️  Setting installed package as editable
🛠 Installed rerun-sdk-0.19.0a1+dev

➜  md5sum ./.pixi/envs/py/lib/python3.11/site-packages/rerun_bindings/rerun_bindings.abi3.so
dde3d5384e3266c6bd14cea18a4345e8  ./.pixi/envs/py/lib/python3.11/site-packages/rerun_bindings/rerun_bindings.abi3.so

And finally run our command again:

➜  pixi run -e py python -c 'import rerun as rr; rr.init("rerun_example_pybuild")'
DEV ENVIRONMENT DETECTED! Re-importing rerun from: /home/jleibs/rerun/rerun_py/rerun_sdk

Expected behavior: should print out "Edited bridge"

Actual behavior: no change.

Now when we check the bindings file again, we note it has been reverted to the copy of the file from BEFORE we ran pixi run py-build

➜  md5sum ./.pixi/envs/py/lib/python3.11/site-packages/rerun_bindings/rerun_bindings.abi3.so
7ca8aa7c0e7a00c8665e7f5be4a828fd  ./.pixi/envs/py/lib/python3.11/site-packages/rerun_bindings/rerun_bindings.abi3.so

This is the major bug. We are now restoring a cached version of the .so from the initial build, even though the rust source has changed.

@jleibs
Copy link

jleibs commented Oct 1, 2024

What do you mean with create just re-install an existing one or create from like an empty installation?

Sorry for the mixup on terminology, I just mean an incremental run, such as:

pixi run -e py python test.py

(I mentally think of that as "Pixi creates some kind of environment context and then runs the command inside of that context". Will try to be more specific though since there's a lot of overloaded terminology here).

@jleibs
Copy link

jleibs commented Oct 1, 2024

isn't a rust compilation something you expect when building the wheel

This is the bit that definitely falls into grey area. Strictly speaking, I agree the "correct / expected" thing to do is to always rebuild the wheel if relevant rust code has changed.

Pragmatically speaking, this can cause the edit+test loop to be significantly slower. Often times you are just editing code that impacts the viewer, but not the bindings, but it can still trigger false-positive re-compiles, which then adds time to running the python test code.

I think my preferred behavior would be to default to correctness (rebuild the wheel if the rust code is edited), but to have some flag or environment that tells pixi to skip those checks and just use the current pixi environment unmodified even if it's in an inconsistent state. This would let power users do the fast thing when they know it's safe enough without creating unexpected behavior for non-power-users.

@tdejager
Copy link
Contributor

tdejager commented Oct 1, 2024

➜ pixi run -e py python -c 'import rerun as rr; rr.init("rerun_example_pybuild")'

Ah thanks @jleibs, as always(!), for your excellent reproducer. Okay, so I debugged whats happening. Let's start of with the fact that:

  1. I'm almost certain that a wheel build of a pyproject.toml with a maturin build-backend triggers a rust re-compile of the rust python bindings. The behavior we then see is based on caching behavior and some decision we made for pixi install.

What's happening here?

  1. We made the decision that if we find a installed distribution that is not installed but is managed by us, that we made the decision to re-install. This is the cause of this behavior. We chose to do this, because if we do not, you cannot get the prefix back into the 'correct' state.
  2. Because we assume we've already built the wheel, we re-instate a cached version, essentially putting back the old wheel when this re-install happens.

How can we fix the re-installation?

So @ruben-arts and @jleibs can start to chime in here what we want the correct behavior to be. I see some paths forward:

  1. We could add a PyPI specific run flag --ignore-non-pixi-installer (or some less hideous version of that name), that essentially disables the behavior of 2).
  2. We could change the behavior of 2) and/or add a --force-reinstall flag or something (which would be useful in itself for meson-py backends e.g), that forces re-installation and rebuild of certain packages (uv has something similar). But with 2) behavior changed you can have an environment in an inconsistent state in the mean time, that is, before running the forced reinstallation.

How can we fix 3), the re-instating of the old wheel

With the current editables, I'm not sure we can, because we have no real way of detecting if a build is needed AFAIC. But I might be missing something here. Note that using the: https://github.com/PyO3/maturin-import-hook could help rebuilding on import. But not sure if this is something you would want.

Note, that with our work on pixi build, we might be more flexible in this regard.

@tdejager
Copy link
Contributor

tdejager commented Oct 2, 2024

I'm not sure but maybe a 'pixi run --no-install --locked' might also be an option on the table @ruben-arts? We would need some extra logic to verify that '--no-install' would be allowed, i.e. a valid conda prefix must exist for that environment.

@jleibs
Copy link

jleibs commented Oct 2, 2024

we have no real way of detecting if a build is needed AFAIC.

Agreed this is generally a hard problem, and in our case users running the build step manually is fine (and in fact preferred).

I think the real problem is we have 2 ways of building the package -- one manually via maturin, and one implicitly via pixi. And only the pixi one ends up cached.

Rather than opt out of pixi management, I suspect we'll have more success leaning in and simply giving us a manual mechanism of forcing pixi to rebuild the package (and update its cache). Then we could get rid of our py-build task that calls maturin and replace it with one that just forces the rebuld of that package that already exists within the context of a specific environment.

For example:

pixi force-rebuild -e py rerun-sdk

(or maybe this is a combination of arguments to pixi install or pixi add).

This also alleviates the first annoyance of the slow dev cycle since it means if we don't run the manual task we continue to just use the cached version and everythign is speedy.

The only problem I see that this doesn't resolve is allowing power-users to manually specify custom maturin flags via a manual invocation, but I think that's a fair compromise. Not only is it fairly niche (quite possibly only effects me). I think I can work around this by introducing custom pixi environments that use maturin environment variables or something. Either way, I'm willing to live with the pain there for the time being.

@tdejager
Copy link
Contributor

tdejager commented Oct 3, 2024

@jleibs a force-reinstall makes sense to me. It would also help this issue: #1695

@nichmor nichmor self-assigned this Oct 14, 2024
@nichmor
Copy link
Contributor

nichmor commented Oct 14, 2024

hey @jleibs !

I'm trying to reproduce your example using this commit and latest pixi:
45add8d9cb7c2384062f3b50df24df2f8d385a63

running pixi run -e py python -c 'import rerun as rr; rr.init("rerun_example_pybuild")' will give me following error

DEV ENVIRONMENT DETECTED! Re-importing rerun from: /Users/graf/projects/oss/rerun/rerun_py/rerun_sdk
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/graf/projects/oss/rerun/rerun_py/rerun_sdk/rerun/__init__.py", line 316, in init
    cleanup_if_forked_child()
  File "/Users/graf/projects/oss/rerun/rerun_py/rerun_sdk/rerun/__init__.py", line 366, in cleanup_if_forked_child
    bindings.cleanup_if_forked_child()
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: module 'rerun_bindings' has no attribute 'cleanup_if_forked_child'
Exception ignored in atexit callback: <function rerun_shutdown at 0x163dd5580>
Traceback (most recent call last):
  File "/Users/graf/projects/oss/rerun/rerun_py/rerun_sdk/rerun/__init__.py", line 347, in rerun_shutdown
    bindings.shutdown()
    ^^^^^^^^^^^^^^^^^
AttributeError: module 'rerun_bindings' has no attribute 'shutdown'

if I switch to latest main ( 7d705802ac4dd70b53e5505e9b09128446b3aefe )

I cannot anymore reproduce this situation. It always pick up the change correctly in the rust bindings. The only difference is that .so files are not present in site-packages, but in the rerun_sdk folder and .pth pointer it's created in site-packages ( which I think it's expected because we are in editable mode?)

Please let me know if you can reproduce it using latest pixi ( 0.32.1 ) and what could be the steps to do it on my side. Maybe I'm missing something between the lines

Thanks!

@jleibs
Copy link

jleibs commented Oct 24, 2024

As an update on this: as of pixi 0.34 (likely before but this just happens to be what I tested), things are now working quite well for us.

If anybody else is running into a similar issue with maturin-based bindings, I believe we inadvertently worked around the issue when we added type stubs and a types.py to our project (as in: https://pyo3.rs/v0.22.5/python-typing-hints.html?highlight=__init__.py#if-you-need-other-python-files)

The net result is that in editable mode, maturin now installs our rerun_bindings.so file into the source tree.

  • Pixi doesn't spuriously rebuild the package on incremental runs (presumably since it's editable)
  • Pixi doesn't "repair" the source tree file since it's not part of the pixi env, and whatever invocation we make to update the contents of that lib seem to persist.

I believe we can close this issue now and I'll open a new one in the future if we find a regression or problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working 🐍 pypi Issue related to PyPI dependencies
Projects
None yet
Development

No branches or pull requests

5 participants