Skip to content

docs: simplify python project example#676

Merged
DavHau merged 1 commit intonix-community:mainfrom
moduon:simpler-python-example
Sep 15, 2023
Merged

docs: simplify python project example#676
DavHau merged 1 commit intonix-community:mainfrom
moduon:simpler-python-example

Conversation

@yajo
Copy link
Copy Markdown
Contributor

@yajo yajo commented Sep 13, 2023

Use lib.importTOML, clearer than the previous implementation.

Reuse more information from pyproject.

pypiSnapshotDate = "2023-08-27";
requirementsList = ["setuptools"] ++ pyproject.project.dependencies;
requirementsList =
pyproject.build-system.requires
Copy link
Copy Markdown
Member

@DavHau DavHau Sep 14, 2023

Choose a reason for hiding this comment

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

I think build-system.requires this is not a mandatory field in a pyproject.toml and might be unset. Maybe use build-system.requires or [] instead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It also would mean that all of your build-system.requires would end up the output derivation, polluting the python env there. Unless I am missing something, that's one of the things build-system.requires tries to avoid in the first place.

Happy to have flag for that behavior as long as it documents the consequences, but I think as a default its not the best choice.

Copy link
Copy Markdown
Contributor Author

@yajo yajo Sep 15, 2023

Choose a reason for hiding this comment

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

#676 (comment): You already have setuptools in the example, and it's affected by that same problem. See:

[build-system]
requires = [ "setuptools" ]

So, this is just an example, and in the example it works. However I agree to add that suggestion if you prefer.

#676 (comment): build-system is not required, but requires is required if build-system exists. The problem of build dependencies landing in python derivations exists in nixpkgs too. See NixOS/nixpkgs#170577 for the whole story. This must change upstream indeed. When it does, then dream2nix can adapt (if needed) its calls to buildPythonPackage with one dependencies for buildtime and others for runtime.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we need to worry about build time deps polluting runtime as long as we don't have a mechanism to differentiate such in the lock file. Missing dependencies are worse.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

IMHO it shouldn't be too hard to fix.

We would just need to be able to specify which drvs go only for buildInputs, and the rest will become propagatedBuildInputs.

Actually it's worse the other way around, where all runtime dependencies must be built before a package is built, when in reality all it needs is setuptools to be built. That's what NixOS/nixpkgs#170577 is about.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The problem of build dependencies landing in python derivations exists in nixpkgs too. See NixOS/nixpkgs#170577 for the whole story. This must change upstream indeed. When it does, then dream2nix can adapt (if needed) its calls to buildPythonPackage with one dependencies for buildtime and others for runtime.

I think the change here makes it about as bad as the old nixpkgs solution, which did leak stuff like wheel and setuptool. But that thing is changing even in nixpkgs since ´pypaBuildHook` got introduced.

don't think we need to worry about build time deps polluting runtime as long as we don't have a mechanism to differentiate such in the lock file. Missing dependencies are worse.

So for an example atm it's not too bad, but I think you should add a comment explaining that it's a current limitation of dream2nix and that the side-effect is that you build-requirements will leak into the final derivation - which may or may not be a problem depending on what you are trying to do while building on the example ;)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't see why this would be a problem. It's merely a closure size issue. Fixing this is just an optimization.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

which may or may not be a problem depending on what you are trying to do while building on the example

As mentioned in #684 I would really like to see a breaking example or at least an explanation of what could break before deciding that this is a notable issue.

Currently this example is using dream2nix as best as it can using the current api. Not having the separation between setup and install dependencies is a more general issue. It has been ignored so far altogether (still stuff builds and runs fine) and I don't see why the current change is making this more of an issue than it was before.

If someone wants to put in work to separate install from setup deps, then I'd appreciate a PR. But until then, let's not be held up by this, except if an immediate problem occurs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As mentioned in #684 I would really like to see a breaking example or at least an explanation of what could break before deciding that this is a notable issue

We've discussed this at length before :D You even saw breaking examples due to the mixup of build and runtime dependencies, i.e. when pinning specific releases of "wheel" or "packaging" there. It's mostly collision.

I don't see why the current change is making this more of an issue than it was before.

Don't think I ever claimed that it's "more of an issue than it was before"?
All I wanted to suggest is not put work arounds for current limitations uncommented in examples.

The idea here in general was not block anything - how could I? This thing is long merged -
but to give newer users an idea of what works like intended and what doesn't at the moment to hopefully slow down the endless churn of small "fixes" necessary in upper layers.

Copy link
Copy Markdown
Member

@DavHau DavHau Sep 18, 2023

Choose a reason for hiding this comment

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

Some things seem to get mixed up here. The problem we had with the wheel package seems unrelated. It simply was not possible to build the wheel package with the underlying nixpkgs builder. It didn't matter if wheel is declare as a build time or run time dependency.

Within dream2nix the pip resolver ensures that all dependencies are conflict free. Therefore simply feeding build-system.requires into the resolver is the safest thing to do with the current architecture. Once the pip resolver succeeds, there won't be any conflicts during runtime, even if all build time dependencies are propagated.

The only exception where conflicts can happen is if the underlying nixpkgs build system leaks its own dependencies, in which case it should be fixed upstream at nixpkgs (like with NixOS/nixpkgs#254547) or worked around downstream, as I did with wheel.

Use `lib.importTOML`, clearer than the previous implementation.

Reuse more information from `pyproject`.
@yajo yajo force-pushed the simpler-python-example branch from 4e1afc4 to 3de6990 Compare September 15, 2023 08:21
@DavHau DavHau merged commit 5a729cb into nix-community:main Sep 15, 2023
@yajo yajo deleted the simpler-python-example branch September 15, 2023 08:42
yajo added a commit to moduon/dream2nix that referenced this pull request Sep 15, 2023
Follow-up of nix-community#676 (comment), now dependencies marked as buildInputs won't be included as propagatedBuildInputs for python derivations.
yajo added a commit to moduon/dream2nix that referenced this pull request Sep 20, 2023
Follow-up of nix-community#676 (comment), now dependencies marked as buildInputs won't be included as propagatedBuildInputs for python derivations.
yajo added a commit to moduon/dream2nix that referenced this pull request Sep 22, 2023
Follow-up of nix-community#676 (comment), now dependencies marked as buildInputs won't be included as propagatedBuildInputs for python derivations.
DavHau pushed a commit that referenced this pull request Sep 22, 2023
Follow-up of #676 (comment), now dependencies marked as buildInputs won't be included as propagatedBuildInputs for python derivations.
chaoflow pushed a commit to chaoflow/dream2nix that referenced this pull request Nov 16, 2024
Follow-up of nix-community#676 (comment), now dependencies marked as buildInputs won't be included as propagatedBuildInputs for python derivations.
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.

3 participants