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

Inconsistent handling of include / exclude for sdist and wheel #8994

Closed
4 tasks done
RobbeSneyders opened this issue Feb 20, 2024 · 10 comments
Closed
4 tasks done

Inconsistent handling of include / exclude for sdist and wheel #8994

RobbeSneyders opened this issue Feb 20, 2024 · 10 comments
Labels
kind/bug Something isn't working as expected status/triage This issue needs to be triaged

Comments

@RobbeSneyders
Copy link

RobbeSneyders commented Feb 20, 2024

  • I am on the latest stable Poetry version, installed using a recommended method.
  • I have searched the issues of this repo and believe that this is not a duplicate.
  • I have consulted the FAQ and blog for any relevant entries or release notes.
  • If an exception occurs when executing a command, I executed it again in debug mode (-vvv option) and have included the output below.

Issue

When building our package, we want to include only yaml files and ignore all other files from a certain subdirectory. The relevant part of our file tree looks like this:

├── src
|   ├── fondant
|   |   ├── components
|   |   |   ├── component_1
|   |   |   |  ├── fondant_component.yaml
|   |   |   |  └── bunch of other files and dirs
|   |   |   └── component_2
|   |   |   |  ├── fondant_component.yaml
|   |   |   |  └── bunch of other files and dirs
|   |   ├── some other directories
|   |   └── some python files
└── pyproject.toml

See our full file tree in our repo, the relevant directories and files are called the same.

We only want to include the fondant_component.yaml file from each component in src/fondant/components and ignore all other files.

I tried to do this with the following include / exclude in our pyproject.toml:

include = ["src/fondant/components/**/*.yaml"]
exclude = ["src/fondant/components"]

So excluding the full directory, and including the specific files we do want to package.

However this leads to inconsistent results in the sdist and wheel outputs:

sdist

src
├── fondant
    ├── components
    |   ├── component_1
    |   |  ├── fondant_component.yaml
    |   └── component_2
    |      ├── fondant_component.yaml
    ├── some other directories
    └── some python files

This is the result we want to achieve.

wheel

├── fondant
|   ├── some other directories
|   └── some python files
└──src
   └── fondant
       └── components
           ├── component_1
           |  ├── fondant_component.yaml
           └── component_2
              └── fondant_component.yaml

As a reproducible example, you can run poetry build from the root of our repo.

I've tried different things to achieve this result:

but none of them worked.

The difference in behavior of include / exclude between sdist and wheel seems like a bug to me, but we would also be interested in a workaround to achieve what we are looking for.

@RobbeSneyders RobbeSneyders added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Feb 20, 2024
@dimbleby
Copy link
Contributor

this difference is per the docs, or anyway it will be in the upcoming release - #8852

please close

@RobbeSneyders
Copy link
Author

I'm not convinced that that is the same issue, or even that the new documentation is correct. In our case include does seem to work for both sdist and wheel without specifying the format explicitly.

If this was the issue, then I would expect the following include / exclude to fix it:

include = [{ path = "src/fondant/components/**/*.yaml", format = ["sdist", "wheel"] } ]
exclude = ["src/fondant/components"]

But the result is unchanged.

@RobbeSneyders
Copy link
Author

This seems closer related to #7153.

include does not seem to work correctly for wheels when using a project layout with a src directory. Because it does include the files, but it includes them under the wrong path.

@dimbleby
Copy link
Contributor

#7153 shows no bug and should be closed

when you say that you want to include src/whatever, poetry takes you at your word and includes it. I don't know a way to say that you want to include-and-move src/whatever

@RobbeSneyders
Copy link
Author

I do think #7153 shows a bug, as do other users based on the comments.

Poetry's behavior is currently inconsistent:

  • for include between sdist and wheel
    See the example I added in my original message. The include path is added as a different path in sdist and wheel. This leads to a working installation with sdist and a broken installation with wheel.
  • for wheel between include and exclude
    See again my original example. I define both my include and exclude paths with src as the root. For exclude, the fact that the fondant package is located in the src directory is correctly taken into account, and fondant/components is excluded. For include, this is not taken into account, and src/fondant/components is included instead.

@dimbleby
Copy link
Contributor

if you want to argue about #7153, please do it in that issue

as we started: that include applies differently to sdist and wheel is known and documented

I don't know whether you've found a new wrinkle where saying "exclude" activates the "include" or something like that, maybe. Feel free to experiment and tease out what is going on, and even submit a pull request: the code is round about here

@RobbeSneyders
Copy link
Author

if you want to argue about #7153, please do it in that issue

You're right. I did not find that issue before opening this one. I will move the discussion there, but would appreciate it if you could actually look into the arguments provided by myself and other users, or at least providing your own clear argumentation instead of being dismissive.

as we started: that include applies differently to sdist and wheel is known and documented

The documented difference is not correct though. It says that include is not applied by default to wheel, while it is applied. Just incorrectly.

@dimbleby
Copy link
Contributor

In the other one, you asked me to confirm that this one is a bug. (Aside: can we please discuss issue X in issue X, and issue Y in issue Y?)

I don't think there's a definitive answer, sorry. I don't understand exactly what the code is doing, why it is doing that, or whether it was intended to do that. If you're able to propose a definite improvement, I assume maintainers would be open to accepting it.

RobbeSneyders added a commit to ml6team/fondant that referenced this issue Feb 20, 2024
The new packaging strategy introduced in #849 doesn't work correctly for
wheels due to a bug in poetry
(python-poetry/poetry#8994), which breaks
installing both from Github and PyPi. This PR introduces a workaround.

Since the bug is related to the `src` directory, this workaround adds a
`pre-build/sh` script which unpacks the `src` directory before building,
and a `post-build.sh` script which moves the package back into the `src`
directory afterwards.

This leads to the following result:
- Installing from Github leads to an installation including all
component files
- Installing locally without running the build script leads to an
installation including all component files
- Installing from PyPi leads to an installation with only the
`fondant_component.yaml` files for each component

In the ideal case, we get the same result for all installation methods,
but this already at least leads to a working result for each method.
@Secrus
Copy link
Member

Secrus commented Oct 6, 2024

Superseded by #9691

@Secrus Secrus closed this as not planned Won't fix, can't repro, duplicate, stale Oct 6, 2024
Copy link

github-actions bot commented Nov 6, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Something isn't working as expected status/triage This issue needs to be triaged
Projects
None yet
Development

No branches or pull requests

3 participants