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

Support the empty-string extra #1503

Closed
wants to merge 1 commit into from
Closed

Conversation

di
Copy link
Sponsor Member

@di di commented Sep 25, 2018

Summary of changes

Allows users to specify an "empty string extra", whose dependencies which will be included in a package's dependencies only if no extras have been requested (i.e., by default).

Closes #1139.

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details

@pganssle
Copy link
Member

pganssle commented Sep 25, 2018

This will be super useful, but I think this should probably be documented explicitly.

@benoit-pierre
Copy link
Member

I'd rather see this as part of a PEP update.

@benoit-pierre
Copy link
Member

This does not work, generated wheels don't have the correct metadata (because an empty extra is not possible):

> cat setup.py
from setuptools import setup


setup(
    name='myproject', version='1.0',
    install_requires='python-xlib',
    extras_require={
        'aiohttp': '''
        aiohttp
        ''',
        'tornado': '''
        tornado
        ''',
        'empty': '''
        ''',
        '': '''
        requests
        ''',
    }
)
> python setup.py -q bdist_wheel && unzip -p dist/myproject-1.0-py3-none-any.whl '*/METADATA'    
Metadata-Version: 2.1
Name: myproject
Version: 1.0
Summary: UNKNOWN
Home-page: UNKNOWN
License: UNKNOWN
Platform: UNKNOWN
Provides-Extra: aiohttp
Provides-Extra: empty
Provides-Extra: tornado
Requires-Dist: python-xlib
Requires-Dist: requests
Provides-Extra: aiohttp
Requires-Dist: aiohttp; extra == 'aiohttp'
Provides-Extra: empty
Provides-Extra: tornado
Requires-Dist: tornado; extra == 'tornado'

UNKNOWN

Also, how would you tell pip to install only the core dependencies? Apart from the project declaring an explicit empty "no_frills" extra.

@di
Copy link
Sponsor Member Author

di commented Sep 25, 2018

I'd rather see this as part of a PEP update.

Are you suggesting an update to PEP 508? If so I think I agree, the specification around extras could stand to be defined a little more strictly (e.g., it doesn't currently specify what makes a valid or invalid extra) and doesn't say anything about this edge case.

This does not work, generated wheels don't have the correct metadata (because an empty extra is not possible):

Yes, we'd need to make an update to wheel as well. Using my example from #1139 (comment):

from setuptools import setup

setup(
    name='foo',
    version='1.0',
    extras_require={
        'aiohttp': 'aiohttp',
        'tornado': 'tornado',
        '': 'requests',
    },
)

It would generate this metadata instead:

Metadata-Version: 2.1
Name: foo
Version: 1.0
Summary: UNKNOWN
Home-page: UNKNOWN
License: UNKNOWN
Platform: UNKNOWN
- Requires-Dist: requests
+ Provides-Extra:
+ Requires-Dist: requests; extra == ''
Provides-Extra: aiohttp
Requires-Dist: aiohttp; extra == 'aiohttp'
Provides-Extra: tornado
Requires-Dist: tornado; extra == 'tornado'

UNKNOWN

At least as far as wheel's metadata format goes, I think an empty extra is possible:

>>> from email.parser import Parser
>>> from io import StringIO
>>> Parser().parse(StringIO('Provides-Extra:')).get('Provides-Extra')
''

The packaging library parses the empty string extra as equivalent to providing no extra at all:

>>> import packaging.requirements
>>> packaging.requirements.Requirement("foo[]").extras
set()
>>> packaging.requirements.Requirement("foo").extras
set()

it's just a matter of properly propagating it through the metadata, and defining the behavior in various bits of tooling.

Also, how would you tell pip to install only the core dependencies?

You wouldn't, presumably any package specifying an "empty string extra" would intend for those requirements to be a part of the core dependencies

@benoit-pierre
Copy link
Member

How do you handle this case:

extras_require={
';"linux" in sys_platform': ['package'],
}
'''

@di
Copy link
Sponsor Member Author

di commented Sep 25, 2018

I think currently that extra would turn into something like:

Requires-Dist: package; "linux" in sys_platform

Which means that it's equivalent to putting it in install_requires and is always going to be installed when the condition matches. Thus, it isn't really an extra at all. With the change I'm proposing, this would become:

Requires-Dist: package; ("linux" in sys_platform) and extra == ''

Which means that it would only be installed for the "empty string extra", and wouldn't be installed if the user specified a different extra.

So technically the behavior would change, however, if the maintainer actually wanted that dependency to be installed across all possible extras, why not just put it in install_requires instead?

@di
Copy link
Sponsor Member Author

di commented Sep 25, 2018

Here's the necessary changes to wheel to support this as described: https://github.com/pypa/wheel/compare/master...di:empty-string-extra?expand=1

@pganssle
Copy link
Member

I am loathe to suggest this, because I don't feel like mailing list discussions are always super productive, but should we kick this over to distutils-SIG, or into a canonical issue somewhere?

FWIW, my use case for this is actually that I would like to implement "negative dependencies", so dateutil would, by default, depend on dateutil.tzdata as a time zone data provider, but you could install dateutil[no-tzdata] to remove the tzdata dependency. I could maybe sorta hack it together using this, but I'd really like a way to do a real negative dependency.

I'm wondering if we can solve the more general problem by allowing extras to declare a replacement either for a package or a set of packages, e.g.:

from setuptools import setup

setup(
    name='foo',
    version='1.0',
    install_requires=['requests'],
    extras_require={
        'aiohttp': {'requests': 'aiohttp'},
        'tornado': {'requests': 'tornado'}
    },
)

This would allow me to declare:

extras_require={
    'no-tzdata': {'dateutil.tzdata': ''}
}

Note: This is not a concrete proposal for an API, I can already see some major objections to what I just suggested, I'm just thinking that it may be time to draft up a PEP (or whatever the current process ends up being) for discussion on this API.

@di
Copy link
Sponsor Member Author

di commented Sep 25, 2018

@pganssle I don't see why what I'm proposing wouldn't work as a negative dependency:

setup(
    name='foo',
    version='1.0',
    install_requires=[...]  # Doesn't include dateutil.tzdata
    extras_require={
        '': ['dateutil.tzdata'],
        'no-tzdata': [],  # Maybe this has a replacement for dateutil.tzdata?
    },
)

IMO, this is cleaner than being able to declare replacements (and would be a lot easier to implement, as the "empty string extra" pretty much fits into the existing way that extras are implemented with minimal changes).

I am loathe to suggest this, because I don't feel like mailing list discussions are always super productive, but should we kick this over to distutils-SIG, or into a canonical issue somewhere?

If I'm going to be making a PEP update to support this, I'll raise the changes to distutils-sig with the draft. I agree that the current "correct" behavior here is more or less undefined and could stand to be specified (even if it's "empty string extras are invalid").

@benoit-pierre
Copy link
Member

Requirements with environment markers do end up in extras_require, always (that's what setuptools does internally).

@di
Copy link
Sponsor Member Author

di commented Sep 25, 2018

I see, so what you're saying is that:

from setuptools import setup

setup(
    name='foo',
    version='1.0',
    install_requires=[
        'package_a;"linux" in sys_platform',
        'package_b',
    ],
)

will produce a requires.txt like:

package_b

[:"linux" in sys_platform]
package_a

And this would create a breaking change for anyone who specified extras, since package_a wouldn't get installed.

I would have expected it to produce a requires.txt like this instead:

package_a; "linux" in sys_platform
packabe_b

As far as I can tell, that was the original behavior (and in fact, pip will correctly respect such conditionals in a source distribution).

It seems like you added this in #1081 because the conditionals were getting stripped when building a wheel. Why didn't that get fixed downstream in wheel instead?

@benoit-pierre
Copy link
Member

The format of requires.txt is not the same as a pip requirement files.

@benoit-pierre
Copy link
Member

The original change (allowing environment markers unfiltered in requires.txt) was a backward incompatible one, and resulted in 2 different ways to declare the same requirements.

@di
Copy link
Sponsor Member Author

di commented Sep 25, 2018

The format of requires.txt is not the same as a pip requirement files.

Yes, I'm aware.

The original change (allowing environment markers unfiltered in requires.txt) was a backward incompatible one, and resulted in 2 different ways to declare the same requirements.

Would you mind going into more detail here? How does "moving" conditional requirements into extras fix this?

@benoit-pierre
Copy link
Member

Because

package_b

[:"linux" in sys_platform]
package_a

is the proper format?

@benoit-pierre
Copy link
Member

install_requires=['package_a; "linux" in sys_platform'],
extras_require={':"linux" in sys_platform': ['package_a']},

and

extras_require={'': ['package_a; "linux" in sys_platform']},

are all equivalent. They all result in the same wheel metadata.

@di
Copy link
Sponsor Member Author

di commented Sep 25, 2018

Sure, what I'm trying to say is that it seems to me like they shouldn't be equivalent, at least it's not what I would have expected before trying it myself. I would have expected that using install_requires to produce only core dependencies:

package_a; "linux" in sys_platform

and would have expected that using extras_require would have produced an extras section:

[:"linux" in sys_platform]
package_a

There seems to be some suprise/confusion about this elsewhere as well, such as here: pex-tool/pex#545 (comment)

I haven't ever seen a specification for the requires.txt file, is it so strict as to disallow conditionals to be included with dependency names?

@benoit-pierre
Copy link
Member

There's no formal specification for requires.txt...

@di
Copy link
Sponsor Member Author

di commented Sep 25, 2018

OK, so aside from the fact that wheel was stripping conditionals, is there any other reason why the original behavior before #1081 was merged is problematic? I'm trying to fully understand the motivation for that change.

(BTW, thanks for all your responses so far!)

@benoit-pierre
Copy link
Member

benoit-pierre commented Sep 25, 2018

There has been 3 behaviours:

  1. the original one: requirements in install_requires with environment markers were not supported, which was not problematic, as using extras_require={':"linux" in sys_platform': ['package_a']}, was how you declared such conditional dependencies
  2. then at some point, this was changed, resulting in a backward incompatible change to the (undocumented) format of requires.txt
  3. this was fixed with Fix environment markers handling #1081, by making sure there's only one way to write requires.txt, even if yes, there are 3 different ways to declare those requirements with setuptools (pretty horrible, although only 2 of them are really compatible with older versions)

In any case, I think the fact that your proposed patch would change the behaviour of some existing requirements is reason enough to not do it like that.

I think ideally you'd want an additional metadata field, specifying the default list of extras.

@benoit-pierre
Copy link
Member

Really, I think the fact that pip supported 2 was just an happy accident (using the same standard API to parse requirements).

@di
Copy link
Sponsor Member Author

di commented Sep 25, 2018

In any case, I think the fact that your proposed patch would change the behaviour of some existing requirements is reason enough to not do it like that.

I agree, and I'm definitely not trying to introduce a breaking change here. It's unfortunate that this is the current status quo, because otherwise this would have been a pretty clean change to apply.

There might be a way to understand the difference between packages produced before this change and after this change. Ideally, a package using the empty-string-extra would say that it provides the empty string as an extra:

Provides-Extra:
Requires-Dist: requests; extra == ''

This isn't currently the behavior of setuptools or wheel, so presumably we could modify installers to understand that a package declaring that it provides the empty-string-extra intends to use the "new" behavior, but that still doesn't cover folks trying to install newer packages with old installers.

@pganssle
Copy link
Member

pganssle commented Sep 26, 2018

@di For most use cases, it would probably be fine as a negative dependency (which is why it was initially exciting to me), but it kinda falls down when you have more than one extra.

Sticking with the real-life example of dateutil, I'm planning on adding an optional compiled back-end to dateutil, (which I'd eventually also like to make opt-out), but while it's opt-in, what I'd like is:

python-dateutil -> ['dateutil.tzdata']
python-dateutil[no-tzdata] -> []
python-dateutil[rust-backend] -> ['dateutil.backend.rust', 'dateutil.tzdata']
python-dateutil[rust-backend, no-tzdata] -> ['dateutil.backend.rust']

In the end it may be that using "negative dependencies" like this ends up being not as helpful as one would hope, because by default if you have any other dependencies that depend on python-dateutil and haven't bothered to opt out, they'll pull in this stuff that you were explicitly trying not to pull in for whatever reason, so a lot of this may not be terribly relevant.

I think the main thing I wanted to mention here was that this is a nice hack, but once you have more than one of these things where you can replace the default backend, you're back to being stuck. That said, just because the solution doesn't generalize doesn't mean we shouldn't implement it, assuming it's not hurting anything to have it there.

@pganssle
Copy link
Member

Actually, now that I think about it, I realized that this may in fact generalize better than I thought if you just add an opt-in flag for the stuff that is initially opt-out, so:

setup(
    name='foo',
    version='1.0',
    install_requires=[...]  # Doesn't include dateutil.tzdata
    extras_require={
        '': ['dateutil.tzdata'],
        'tzdata': ['dateutil.tzdata'],
        'no-tzdata': [],  # Maybe this has a replacement for dateutil.tzdata?
        'rust-backend': ['dateutil.backend.rust'],
    },
)

This is not ideal, but at least it makes the combinatorial logic I wanted possible, with:

python-dateutil -> ['dateutil.tzdata']
python-dateutil[no-tzdata] -> []
python-dateutil[rust-backend, tzdata] -> ['dateutil.backend.rust', 'dateutil.tzdata']
python-dateutil[rust-backend, no-tzdata] -> ['dateutil.backend.rust']

@di
Copy link
Sponsor Member Author

di commented Oct 3, 2018

Unfortunately I don't see a way this approach is going to work given the current state of things, without breaking backwards compatibility. As such I'm going to close this PR.

Thanks for the discussion @benoit-pierre and @pganssle!

@kousu
Copy link

kousu commented Jan 1, 2023

Thanks for trying @di. It's a lot of work to manage a world-wide piece of software. So thanks for trying to improve it. And thanks to @pganssle for taking the time to examine the pros and cons. This discussion has been a useful reference for me.

Do you think https://speakerdeck.com/uranusjr/lets-fix-extras-in-core-metadata-3-dot-0 will be able to address what you were hoping to address here?

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.

4 participants