prefix-dev / pixi

Package management made easy
https://pixi.sh
BSD 3-Clause "New" or "Revised" License
3.29k stars 182 forks source link

Incorrectly rebuilding editable pypi dependency, and not installing it #1466

Open rahuldave opened 5 months ago

rahuldave commented 5 months ago

Checks

Reproducible example

See the repo https://github.com/rahuldave/projpackname

Issue description

We start with:

[project]
name = "projpackname_rd"

and

[tool.setuptools]
packages = ["projpackname"]
package-dir = { "" = "." }

Now if we do:

[tool.pixi.pypi-dependencies]
projpackname = { path = ".", editable = true }

with this, the lockfile/editable setup is always rebuilt, and we have

❯ grep projpack pixi.lock
 name: projpackname-rd

tested by doing pixi run ls in the default environment

Now, change to

[tool.pixi.pypi-dependencies]
projpackname_rd = { path = ".", editable = true }

to suppress the rebuild (one rebuild will happen but no subsequent rebuilds), once again tested by doing tested by doing pixi run ls in the default environment. The grep gives the same result:

❯ grep projpack pixi.lock
  name: projpackname-rd

Expected behavior

Not sure.

tdejager commented 5 months ago

This is indeed a bug and I can confirm that we can reproduce. There is a mismatch in the satifisfiability where it expects the lock to contain the name of the editable dependency, but it does not, triggering a resolve and re-build. However we need to think of a good way to fix this.

We could:

  1. Disallow the dependency having a different name then the project. We would need to be able to verify this in the satisfiability, this requires reading the pyproject.toml or worse building the setup.py and this is pretty slow. We do cache the setup.py though so we would only need to do this when the file changes.
  2. Save the name of the dependency as the name of the package in the lock file, the downside of this is that we need to do some cross-referencing in the code. Also this does not need to be the actual name of the project.
  3. Save an alias for the name in the lock file, this is flexible but might be a bit ugly.

I've done some research and poetry disallows having a different name for the project and the dependeny, so I suppose that's the prior art here as their syntax in the toml is very close to ours.

tdejager commented 5 months ago

Would love to hear other peoples opinion about this. @olivier-lacroix or others :)

tdejager commented 5 months ago

Also, like @ruben-arts added, for 1) you need to have a conda environment (with python) ready to do an actual build something you want to avoid in the validation step which should simply be a function that checks if the lock satifies the requested dependencies (i.e. whatever is in the manifest file).

tdejager commented 5 months ago

Another way to trigger:

Use this pixi.toml:

[project]
name = "rename-flask"
version = "0.1.0"
description = "Add a short description here"
authors = ["Tim de Jager <tim@prefix.dev>"]
channels = ["conda-forge"]
platforms = ["osx-arm64"]

[tasks]

[dependencies]
python = ">=3.12.3,<3.13"

[pypi-dependencies]
flask = { url = "https://files.pythonhosted.org/packages/41/e1/d104c83026f8d35dfd2c261df7d64738341067526406b40190bc063e829a/flask-3.0.3.tar.gz" }

Run: pixi install

Change the dependency in the toml to:

[pypi-dependencies]
hello = { url = "https://files.pythonhosted.org/packages/41/e1/d104c83026f8d35dfd2c261df7d64738341067526406b40190bc063e829a/flask-3.0.3.tar.gz" }

And the environment will keep being unsatisfied, triggering re-installs.

olivier-lacroix commented 5 months ago

So, shooting a bit from the hip here, but my understanding is that the setuptools packages argument contains the name of the importable module, while the project name refers to the installable package name.

If that is the case, and unless I am missing something, the current behaviour seems correct?

rahuldave commented 5 months ago

The crucial piece to not rebuild is

[tool.pixi.pypi-dependencies]
projpackname_rd = { path = ".", editable = true }

imports then wotk with import projpackname

image

So i am not sure this behavior, as @olivier-lacroix saya is erroneus. But i had originally just used projpackname rather than projpackname_rd above, and that triggered continuous rebuilds...

rahuldave commented 5 months ago

Ok so the plot gets a bit thicker: i am able to run tests locally without an issue, but when i try to run them in ci/cd i run into an issue. It comes from here in wikiapp/rikiapp.__init__.py:

__version__ = importlib.metadata.version(__name__ or __package__)

Now on my laptop __name__ and __package__ get set to wikiapp, not wikiapp_rd. But in ci/cd i believe we install from lockfile, and in the lockfile we dont have wikiapp, but rather wikiapp-rd. This causes the above statement to error out.

E   importlib.metadata.PackageNotFoundError: No package metadata was found for wikiapp
[58](https://github.com/rahuldave/wikiapp/actions/runs/9325628257/job/25672982734#step:4:59)

So while this is the right behaviour i suspect it might conflict with canonical pixi behaviour (--locked)

olivier-lacroix commented 5 months ago

My hunch is this is an unrelated issue, where locally your import works because or file system resolution, but in CI/CD, this may fail

rahuldave commented 5 months ago

I dont think so? When I run the tests both locally and in CI/CD with: name = "wikiapp_rd" changed to name = "wikiapp" in the [project] section, AND

[tool.pixi.pypi-dependencies]
wikiapp_rd = { path = ".", editable = true }

changed to

[tool.pixi.pypi-dependencies]
wikiapp = { path = ".", editable = true }

while in both cases retaining:

[tool.setuptools]
packages = ["wikiapp"]
package-dir = { "" = "." }

my tests pass in ci/cd (and locally)

printing __name__ and __package__ on ci/cd have them both resolve to wikiapp means that my original hypothesis that they were being set to wikipedia-rd is WRONG. But something about the change still borks metadata resolution...is dev installation skipped in ci/cd cause that would be needed for this to work i think? (and is perhaps not needed when there is a package-name/import name identicality)..

tdejager commented 5 months ago

I don't know whats happening here, but there can be a difference between a build that has been done before (cached results would be used) and a build without a cache.