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

Poetry export should use per-package --index-url instead of single --extra-index-url #37

Closed
3 tasks done
jaklan opened this issue Oct 22, 2020 · 23 comments
Closed
3 tasks done
Labels
wontfix This will not be worked on

Comments

@jaklan
Copy link

jaklan commented Oct 22, 2020

  • I am on the latest Poetry version.
  • I have searched the issues of this repo and believe that this is not a duplicate.
  • If an exception occurs when executing a command, I executed it again in debug mode (-vvv option).

Issue

This issue is a follow-up to the problem identified in python-poetry/poetry#3238:

But... there's one more thing. I've also run:
poetry@3251 export -f requirements.txt -o requirements.txt --without-hashes --with-credentials
and the first line of the output file is:
--extra-index-url http://repository.intranet.foobar.com/artifactory/api/pypi/pypi-virtual/simple

When using default = true we get then:
--index-url http://repository.intranet.foobar.com/artifactory/api/pypi/pypi-virtual/simple

At first sight it seems okay, but we have to remember about the issue with pip and --extra-index-url:
https://pydist.com/blog/extra-index-url
pypa/pip#5045
I assume it's already handled by Poetry itself, but to make it error-prone when exporting the requirements.txt file imho we should also keep the approach of specifying individual index per each package, e.g.:
--index-url http://repository.intranet.foobar.com/artifactory/api/pypi/pypi-virtual/simple anyconfig==0.9.11

PS Of course there should be also --trusted-host flag in any of these scenarios due to http, but there was another PR for that, afair - already merged, just not released yet.

@abn has already confirmed the validity of the issue, but also provided one concern:

@jaklan appreciate the input on that. I'd suggest we raise that as a new issue. I agree that a per package index is better for the extra-index case. One worry, however, is that people seem to be attached to the current export format because some do text processing of the output it for various reasons.

So we also have to consider how to introduce such a change not to break any existing workflows.

@sinoroc
Copy link

sinoroc commented Oct 26, 2020

So we also have to consider how to introduce such a change not to break any existing workflows.

I guess a new flag --without-indexes could help, maybe?

@estyxx
Copy link
Member

estyxx commented Oct 26, 2020

I can work on that!
But I'm confused on the name of the flag: if atm we don't have per package index and we don't want to change this default, think the flag should be --with-indexes, means that the export will add --index-url http://repository.intranet.foobar.com/artifactory/api/pypi/pypi-virtual/simple before the dependency...

@sinoroc
Copy link

sinoroc commented Oct 26, 2020

@estyxx I'm sure it'd be much appreciated if you could get a PR started.

Maybe you're right, a --with-indexes flag might make more sense, if we want to keep the current default behaviour.

It was just a suggestion though, the finer details/requirements of the feature will appear as the implementation goes. Or would you rather have a more concrete outline of what the feature should look like first? We can brainstorm on that.

@estyxx
Copy link
Member

estyxx commented Oct 26, 2020

@sinoroc
I was thinking to add a parameter in the Exporter class with_indexes and check here (exporter.py#L90) as it is already checking if the package has source_url...
so should look something like:


            if not is_direct_reference and package.source_url:
                if with_indexes:
                    line = f"--index-url {package.source_url} " + line

what do you think?

@sinoroc
Copy link

sinoroc commented Oct 26, 2020

@estyxx

I do not know the code base, to be able to judge on the implementation. I was suggesting we brainstorm on what the UX should look like and what the output should look like for some particular examples of inputs. But looks like you already have a PR ready, so I'm a bit too slow... :D

For example:

  • I wonder if the output still needs the --extra-index-url statements at the top. I would say no.
  • how do we deal with credentials?

@estyxx
Copy link
Member

estyxx commented Oct 26, 2020

@sinoroc
Sorry I was thinking you were one of the maintainer/contributors... was an easy fix, but I converted the PR in Draft so we can discuss...

I wonder if the output still needs the --extra-index-url statements at the top. I would say no.

Think this option could be omitted now that each package has his --index-url

how do we deal with credentials?

Think I have to check if there is the "authenticated URL" in the package object, and put that one instead in that case

@sinoroc
Copy link

sinoroc commented Oct 26, 2020

I am re-reading the "Requirements File Format" section in pip's documentation:

The options which can be applied to individual requirements are --install-option, --global-option and --hash.

Are we sure that the output we are trying to generate here is conform?

@jaklan
Copy link
Author

jaklan commented Oct 26, 2020

@sinoroc It seems to be some understatement in docs, because as far as I remember per-package flags simply work, but it would be good to a) verify if it's still true with the actual pip version b) create an issue on pip repo to clarify that.

About the naming - I would keep it direct, e.g. --per-package-indices

@sinoroc
Copy link

sinoroc commented Oct 26, 2020

a) verify if it's still true with the actual pip version b) create an issue on pip repo to clarify that.

Yes, please. Let us know.

About the naming - I would keep it direct, e.g. --per-package-indices

Feels like it would be nicer to stay in the spirit of the other flags --without-hashes, --with-credentials.

@jaklan
Copy link
Author

jaklan commented Oct 26, 2020

@sinoroc I get the point, but --with-indices is just misleading - --extra-index-url is also an index anyway ;) We can then go with --with-package-indices.

@abn
Copy link
Member

abn commented Oct 26, 2020

@estyxx appreciate you starting to look into this. 👍

@sinoroc you are correct, the -i option will not work per-package.

Additionally, seems pip by design does not allow for ordering indexes. See:

I do not think this has changed since these issues. The only sure fire way to ensure the right package is used us to rely on a direct origin url. So use the file url explicitly. package @ url.

The only special case we should support is when PyPI is disabled, which I think should already be the case by refering the private the repo as the index (if not - bug?).

The following is what we should discuss first.

  1. Should we support the export of direct origin url instead of PEP 508 implicitly when default = true?
  2. Should we even support trying to fit a "poetry notion" of index preferences into requirements.txt format?

For (2), my prefernce is no. This is because, by doing so we will be encouraging a not so good practice of having same package versions overriden in private indexes. In cases where a proxy is used this should be used as the index and not as an extra index (disabling PyPI).

As for (1), I would think this should be what the flag toggles (--use-pep-610 | --direct-origin). Unfortunately, I am a bit weary of introducing a format specific flag at the moment because we do not know what other export formats we want to support.

@jaklan as I have pointed out above, a per-package option might not be feasible. The only way I can think of to support the custom ordering is using PEP 610.

@sinoroc
Copy link

sinoroc commented Oct 26, 2020

@jaklan I meant we could try to keep a with or without prefix. --with-per-package-index, -with-package-indices, or... I do not know. I do not have the last word anyway.

@sinoroc
Copy link

sinoroc commented Oct 26, 2020

@abn Thanks for the additional info.

Should the output of poetry export be portable? Should a requirements.txt be installable under any Python version on any platform (as long as it is supported by the root project obviously)? In other words: can we really use direct URLS (PEP 508, PEP 440) or direct URL origin (PEP 610)?

Comparatively: are poetry.lock files portable?

If indeed --index-url can not be set on a per requirement basis, I feel like this feature request can not be fulfilled. Unless I am missing something.


[Thinking about all this, I feel like I have already tried to tackle this in a non-poetry context and came to the conclusion that it was not possible]

@estyxx
Copy link
Member

estyxx commented Oct 27, 2020

@abn

Unfortunately, I am a bit weary of introducing a format specific flag at the moment because we do not know what other export formats we want to support.

I don't know what kind of exports poetry is going to support in the future, but think is kind of natural that different exports can have differents flags and options... maybe we should think to validate them based on the export format selected...

direct origin URL seems a good option btw...

@jaklan
Copy link
Author

jaklan commented Nov 4, 2020

@abn thanks for the detailed explanation! Just for the record: I've done some tests and can confirm per-package indices simply doesn't work - I got the impression it was possible in the past, but maybe it was just a false memory ;) Anyway, I agree the solution you proposed seems to be the only reasonable one for the moment, but I also see it's not the most trivial one, so without proper user demand it rather can wait.

Just to quote the workaround for the future viewers from one of the above links:

Rather than registering a whole load of empty packages on PyPI (which as you say, isn't that friendly, although probably OK if you use a unique prefix like mycompany.XXX), you might want to look at using devpi as a local package cache. I believe that allows you to hold your private packages locally, but "pass through" to PyPI for anything not held in your devpi instance. Then you'd use --index-url to point to your local devpi as the package index, and skip using PyPI at all.

PS I agree with @estyxx about not being afraid of format specific flags, it could be hard to avoid them when the next formats are introduced anyway

@sinoroc
Copy link

sinoroc commented Nov 5, 2020

On a side note... As I have already noted in this (unrelated) tox ticket:

I feel like a thing that might be missing in the Python packaging ecosystem is a requirement notation such as:

Library @ https://pep503.tango.dev/simple/

I believe this would fit quite well into requirements.txt files. In my mind it would translate to:

python -m pip install --index-url 'https://pep503.tango.dev/' Library

@sinoroc
Copy link

sinoroc commented Nov 5, 2020

I opened a discussion on the topic:

@sinoroc
Copy link

sinoroc commented Nov 5, 2020

As far as I understood, the philosophy from the point of view of pip, and PyPI (and I guess PyPA ecosystem in general) is that indexes should be indistinguishable, interchangeable. If 2 projects of the same name exist on 2 indexes, it should be assumed that they are the exact same project. And 2 distributions of the same name and version number should be assumed to be the exact same distribution and so it does not matter from which one we fetch. In other words:

Packages are expected to be unique up to name and version, so two wheels with the same package name and version are treated as indistinguishable by pip. This is a deliberate feature of the package metadata, and not likely to change.

-- pfmoore, pypa/pip#5045 (comment)

So for our current issue, it is further proof to me that relying on --index-url and --extra-index-url for an accurate export is not the way to go. pip (i.e. requirements.txt) can not guarantee from which index a particular dependency is going to be fetched (when there are multiple indexes).

[Short of relying on direct URLs Library @ https://dists.tango.dev/library-1.2.3-xyz.whl I do not see how it can be done, right now. But maybe I am missing something obvious.]

If one needs to circumvent this behaviour and regain control over the situation, they need to put something like devpi or pydist in place.

  • In the case of devpi, its "inheritance" feature seems of particular importance for that concern. As far as I understood this would be the key feature that would prevent downloading a dependency from the "wrong" index (not sure how exactly that works and how to do the configuration, though).
  • For pydist: https://pydist.com/blog/extra-index-url
  • Probably also possible in other servers...

On a personal note, I wonder if maybe we should look at all features/issues such as these in a different light:

Either it is somewhat of a "false claim" (a promise that can not be kept, due to the already existing ecosystem). Or poetry could double down on that, double check that the prioritization of indexes works as intended and sell it as an advantage over other tools (pip, etc.).

@jaklan
Copy link
Author

jaklan commented Nov 5, 2020

@sinoroc if I understand correctly the PR introducing experimental.new-installer (which is enabled by default): python-poetry/poetry#2595, then it seems poetry (almost) doesn't rely on pip anymore:

This is the first step towards the removal of calls to pip for installation of packages. We now use it exclusively to install distributions available locally.

so if installer logic is already on the Poetry side, then, I guess, it can claim it ensures the precedence.

@sinoroc
Copy link

sinoroc commented Dec 11, 2020

I link python-poetry/poetry#3477 as somewhat related.
(That is how I should have linked from the beginning and not the other way around.)

@Anovikov9
Copy link

@abn sorry, flag --without-indexes already implemented? I need to get requirements without index-url

@finswimmer finswimmer transferred this issue from python-poetry/poetry Dec 25, 2021
@henrytseng
Copy link

It seems that settings from pyproject.toml aren't getting pulled into requirements.txt either.

[[tool.poetry.source]]
name = "myprivate"
url = "https://myprivate/simple/"
default = true

[[tool.poetry.source]]
name = "default"
url = "https://pypi.org/simple"
secondary = true

Should create a requirements.txt with

--index-url https://myprivate/simple
--extra-index-url https://pypi.org/simple

@neersighted
Copy link
Member

Closing as wontfix given that this is a limitation of the format and not up to Poetry. @henrytseng's issue is different and already fixed.

@neersighted neersighted closed this as not planned Won't fix, can't repro, duplicate, stale Oct 11, 2022
@neersighted neersighted added the wontfix This will not be worked on label Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

7 participants