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

Add ability to specify "default" extras_require #1139

Open
Kentzo opened this issue Sep 1, 2017 · 38 comments
Open

Add ability to specify "default" extras_require #1139

Kentzo opened this issue Sep 1, 2017 · 38 comments
Labels
enhancement Needs Discussion Issues where the implementation still needs to be discussed.

Comments

@Kentzo
Copy link
Contributor

Kentzo commented Sep 1, 2017

Libraries that implement a client to an HTTP backend often provide multiple implementations such as for tornado, aiohttp or requests. Each implementation comes with its own dependencies. User usually needs only one implementation.

With extras_require developer of the library can specify aiohttp and torndao as optional and install requests for everyone via install_requires.

However, users of aiohttp and tornado do not need requests, hence the request to be able to specify a "default" extra that should be picked when no other extras are specified.

So that users calling pip install mylib or pip install mylib[requests] will get requests installed, but users calling pip install mylib[aiohttp] won't.

@benoit-pierre
Copy link
Member

This work with pip:

from setuptools import setup

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

But not when setuptools' easy_install tries to install the necessary requirements (UndefinedEnvironmentName: 'extra'). For the same reason this does not work either:

from setuptools import setup

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

IMHO, both should be allowed. @jaraco: what do you think?

@agronholm
Copy link
Contributor

PEP 508 makes no mention of such behavior, so implementing this would cause setuptools to fall out of compliance, IMHO.

@Kentzo
Copy link
Contributor Author

Kentzo commented Nov 16, 2017

Perhaps environment should provide extra == '' when no extra is specified. That would make the first example by @benoit-pierre to work.

@agronholm PEP 508 specifies the "extra" marker but it doesn't seem to specify its value when it's not set.

@peterbrittain
Copy link

I've just been experimenting with this problem...

This has broken for me since 36.2. Since then, if you use extra inside your install_requires you hit the problem @benoit-pierre describes above. Stack for me looks like this:

       Traceback (most recent call last):
         File "setup.py", line 3, in <module>
           setup(name='foo', zip_safe=False, )
         ...
         File "/home/peter/setuptools/pkg_resources/__init__.py", line 2661, in _dep_map
           if invalid_marker(marker):
         File "/home/peter/setuptools/pkg_resources/__init__.py", line 1441, in invalid_marker
           evaluate_marker(text)
         File "/home/peter/setuptools/pkg_resources/__init__.py", line 1459, in evaluate_marker
           return marker.evaluate()
         File "/home/peter/setuptools/pkg_resources/_vendor/packaging/markers.py", line 301, in evaluate
           return _evaluate_markers(self._markers, current_environment)
         File "/home/peter/setuptools/pkg_resources/_vendor/packaging/markers.py", line 226, in _evaluate_markers
           lhs_value = _get_env(environment, lhs.value)
         File "/home/peter/setuptools/pkg_resources/_vendor/packaging/markers.py", line 208, in _get_env
           "{0!r} does not exist in evaluation environment.".format(name)
       pkg_resources._vendor.packaging.markers.UndefinedEnvironmentName: 'extra' does not exist in evaluation environment.

However, invalid_marker is just trying to check if a marker is valid or not, so I wondered about catching the exception there and just saying it's not valid (by returning the exception rather than False). So I created this patch.

diff --git a/pkg_resources/__init__.py b/pkg_resources/__init__.py
index 7333464..71f614e 100644
--- a/pkg_resources/__init__.py
+++ b/pkg_resources/__init__.py
@@ -1439,6 +1439,8 @@ def invalid_marker(text):
     """
     try:
         evaluate_marker(text)
+    except packaging.markers.UndefinedEnvironmentName as e:
+        return e
     except SyntaxError as e:
         e.filename = None
         e.lineno = None
diff --git a/setuptools/tests/test_egg_info.py b/setuptools/tests/test_egg_info.py
index 66ca916..b2cd384 100644
--- a/setuptools/tests/test_egg_info.py
+++ b/setuptools/tests/test_egg_info.py
@@ -284,6 +284,19 @@ class TestEggInfo(object):
         ''',
 
         '''
+        install_requires_with_extra_marker
+
+        install_requires=["barbazquux; extra != 'foo'"],
+
+        [options]
+        install_requires =
+            barbazquux; extra != 'foo'
+
+        [:extra != "foo"]
+        barbazquux
+        ''',
+
+        '''

Seems to do the trick for me! Would you like me to submit a PR?

@benoit-pierre
Copy link
Member

I don't think this is right. A better alternative would be to always define the extra environment marker, setting it to None if it's not specified. I think that's what pip does.

@peterbrittain
Copy link

Yeah - that would work too. I'm not sure if that aligns with pep 508, though.

The "extra" variable is special. It is used by wheels to signal which specifications apply to a given extra in the wheel METADATA file, but since the METADATA file is based on a draft version of PEP-426, there is no current specification for this. Regardless, outside of a context where this special handling is taking place, the "extra" variable should result in an error like all other unknown variables.

@benoit-pierre
Copy link
Member

Yeah... I don't know why it was specified as such. It makes some things really hard, like converting metadata from wheel to egg: https://github.com/pypa/setuptools/blob/master/setuptools/wheel.py#L105

@peterbrittain
Copy link

Lovely! :-)

I'm still new to the setuptools code, so don't know where the extra variable would need to be defined in a safe way. Happy to have a look at that for a PR instead if that's what's needed, but a pointer would be appreciated...

@benoit-pierre
Copy link
Member

I think this is what I had:

diff --git a/pkg_resources/__init__.py b/pkg_resources/__init__.py
index 6f1071fb..db241642 100644
--- a/pkg_resources/__init__.py
+++ b/pkg_resources/__init__.py
@@ -1456,7 +1456,7 @@ def evaluate_marker(text, extra=None):
     """
     try:
         marker = packaging.markers.Marker(text)
-        return marker.evaluate()
+        return marker.evaluate({'extra': extra})
     except packaging.markers.InvalidMarker as e:
         raise SyntaxError(e)
 
@@ -2678,7 +2678,7 @@ class Distribution(object):
                             if invalid_marker(marker):
                                 # XXX warn
                                 reqs = []
-                            elif not evaluate_marker(marker):
+                            elif not evaluate_marker(marker, extra=extra):
                                 reqs = []
                         extra = safe_extra(extra) or None
                     dm.setdefault(extra, []).extend(parse_requirements(reqs))

@peterbrittain
Copy link

Looks like that's not quite enough... I get this error when I run my test input (from the previous diff) against it.

E AssertionError: warning: install_lib: 'build/lib' does not exist -- no Python modules to install
E
E Couldn't find index page for 'barbazquux' (maybe misspelled?)
E No local packages or working download links found for barbazquux
E error: Could not find suitable distribution for Requirement.parse('barbazquux')

@benoit-pierre
Copy link
Member

That's normal, no? Your test is trying to install a requirement that cannot be met, change it to:

 setuptools/tests/test_egg_info.py | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git i/setuptools/tests/test_egg_info.py w/setuptools/tests/test_egg_info.py
index 66ca9164..b270ba25 100644
--- i/setuptools/tests/test_egg_info.py
+++ w/setuptools/tests/test_egg_info.py
@@ -373,6 +373,20 @@ class TestEggInfo(object):
 
         [empty]
         ''',
+
+        '''
+        install_requires_with_extra_marker
+
+        install_requires=["pytz; extra != 'foo'"],
+
+        [options]
+        install_requires =
+            pytz; extra != 'foo'
+
+        [:extra != "foo"]
+        pytz
+        ''',
+
         # Format arguments.
         invalid_marker=invalid_marker,
         mismatch_marker=mismatch_marker,

@peterbrittain
Copy link

Sorry - I was being dense there. I'm clearly trying to do too much in too little time and making careless mistakes as a result. Is it worth packaging this fix up as a PR?

@benoit-pierre
Copy link
Member

No, this does not work... Because of the way the dependency map is built when using a dist-info distribution (wheel install), pytz will be added to the common requirements (and installed no matter the extra).

@peterbrittain
Copy link

@benoit-pierre Still finding my way around this code... I think you're talking about invoking the Wheel.install_as_egg() path. When I try setting up a test to invoke that, the resulting PKG-INFO file contains:

Requires-Dist: foobar; extra == "bar"

And the metedata.json also has conditional content:

{"description_content_type": "UNKNOWN", "extensions": {"python.details": {"document_names": {"description": "DESCRIPTION.rst"}}}, "extras": [], "generator": "bdist_wheel (0.30.0)", "metadata_version": "2.0", "name": "foo", "run_requires": [{"environment": "extra == \"bar\"", "requires": ["foobar"]}], "summary": "UNKNOWN", "version": "1.0"}

That all looks plausible to me... What have I missed?

@decentral1se
Copy link

Bump @benoit-pierre! Would love to see this pushed forward in some way if you have time.

@di
Copy link
Sponsor Member

di commented Sep 24, 2018

Would using the empty-string extra as a default be a reasonable approach here?

from setuptools import setup

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

I was able to get this to work via setuptools with a two-line change to pkg_resources:

diff --git a/pkg_resources/__init__.py b/pkg_resources/__init__.py
index 3ae2c5cd..2226b8ae 100644
--- a/pkg_resources/__init__.py
+++ b/pkg_resources/__init__.py
@@ -2628,6 +2628,8 @@ class Distribution:
                 raise UnknownExtra(
                     "%s has no such extra feature %r" % (self, ext)
                 )
+        if not extras:
+            deps.extend(dm.get('', ()))
         return deps

     def _get_metadata(self, name):

Presumably the empty-string-extra is not used elsewhere, and running pip install foo[] already succeeds and installs all the same dependencies as pip install foo for the package as one would expect.

@di di mentioned this issue Sep 25, 2018
2 tasks
@di
Copy link
Sponsor Member

di commented Sep 25, 2018

I made a PR: #1503 Not going to work given the current state of things.

@pganssle pganssle added the Needs Triage Issues that need to be evaluated for severity and status. label Oct 19, 2018
@pganssle pganssle added enhancement Needs Discussion Issues where the implementation still needs to be discussed. and removed Needs Triage Issues that need to be evaluated for severity and status. labels Oct 23, 2018
@jonas-eschle
Copy link

jonas-eschle commented May 3, 2021

No, not a single package will break. Just to introduce a new syntax: you can define None in the extras_require as a key which defines requirements that are only additional to the default.
Nothing will break since None is currently not allowed as a key, so no one has it.
So the default is equal to an "extra" None.

if you install any extra, it takes the install_requires and the extra. If no extra is specified, it takes the install_requires and the None extra (trivially: which is empty in case it is not specified).

This makes the default less of a special case, in fact it makes everything more consistent.

facebook-github-bot pushed a commit to facebookresearch/Kats that referenced this issue Aug 17, 2021
…al dependencies

Summary:
The main changes are to `requirements.txt` and `setup.py`.

After much discussion, we prefer that `pip install kats` installs everything, but that power users can have a way to opt out of some of dependencies required in only some parts of Kats (and then manually opt into them as desired). `pip` doesn't directly support this use case (see [setuptools issue](pypa/setuptools#1139), [packaging-problems issue](pypa/packaging-problems#214)), so we had to roll our own solution:

```
MINIMAL=1 pip install kats
```

Optional depedencies moved to `test_requirements.txt`. Two imports (`LunarCalendar`, `convertdate`) were not used anywhere anymore in Kats and removed.

The rest of the changes wrap these now-optional imports in `try/except` blocks. In two cases, the changes are made to `__init__.py` files and log a warning that that the affected functionality is not available at all if the required depedencies aren't there. Otherwise, the modules can be imported but some methods cannot work without the optional dependencies (for example, some plotting methods require `plotly`).

Reviewed By: rohanfb

Differential Revision: D30172812

fbshipit-source-id: 2c7d8072e72cbdd2ac9960fe953cabe15db63cc9
@FirefoxMetzger
Copy link

Allow me to add another +1 to the already long list of +1 posts. I would like to use this in ImageIO (if anyone happens to know it), to default to a backend to install, if no backends are selected explicitly. Currently we always install pillow, but that doesn't work on all platforms.

@dholth What is currently blocking this issue from progressing into a PR? A agreed-upon solution, or the manpower to implement it?

I like the idea of a None field in extras_require (I tried this intuitively, and was surprised that it didn't work), and I am happy to help with the PR/review if that can make things move forward.

@geekscrapy
Copy link

+1 I have a project which is capable of using either of two packages and selects between them at runtime.
So on install, I just need to be able to say: install_requires=['thispkg OR thatpkg']

@nschloe
Copy link

nschloe commented Dec 12, 2021

As the developer of "C", I just bumped into a scenario that requires dependencies that can be turned off. Scenario:

  • A depends on B < 3.0
  • C depends on A and B >= 3.0

It looks like C cannot dependy on both A and a newer version of B. But hold on, B isn't really necessary for A! So I suggest to the developers of A to make B an optional dependency. This is turned down, because users of A would then by default miss the functionality of B.

A solution would be an optional dependency B that in C can be turned off.

@tristanlatr
Copy link

tristanlatr commented Mar 9, 2022

Here is my hack to quickly implement such feature within the setup.py.

from setuptools import setup
import sys

# Hack to implement a default extra require with `lxml` and `bs4` packages that can be replaced with `[minidom]`
is_minidom_extra_enabled = any('minidom' in a for a in sys.argv)

def get_requirements():
    requirements = [   'requests',
                        'aiohttp',
                        'aioresponses',
                        'cached_property', ]
    if not is_minidom_extra_enabled:
        requirements.extend(['beautifulsoup4', 'lxml'])
    return requirements

setup(
    ...
    install_requires    =   get_requirements(),
    extras_require      =   {
                             'minidom': ['dom_query'],
                            },
)

EDIT: Actually, this does not seem to work :/

@Pandede
Copy link

Pandede commented Dec 20, 2023

Any progress?

@bwoodsend
Copy link

I know I'll be shot down for this but please no. We do not need to make dependency trees more ambiguous. This concept of recommended dependencies has already plagued apt and dnf where it leads to junk ridden environments that are several times larger than they should and/or broken environments where a developer thought one of their dependencies was optional but hadn't tested that permutation and it accidentally became mandatory. If you have interchangeable dependencies, just pick one. If you have dependencies that you don't need then don't call them dependencies. If you think your packages couples well with some other package, just say so in the documentation -- don't dump it into their environment without telling them. There are many tempting uses for this feature but none of them will improve your user or developer experiences.

@luckydonald
Copy link

My use case is to have a default implementation, but offer alternatives backends.

think requests vs httpx.
Requests is a sane default for much uses, and hence makes sense as dependency for a battery-included install. However, I'd like to give the option to install httpx or something else instead - in that case without forcing the user to install requests additionally.

@bwoodsend
Copy link

... so your users get a less consistent experience, any asymmetries between requests and httpx could cause a pip install requests to break their code, and you now have 4 packaging permutations to verify work sensibly rather than 1. That's not a feature. Just pick one.

@jab
Copy link

jab commented Sep 14, 2024

Sounds like you’re assuming a poor implementation of what @luckydonald described, which, to be fair, it’s easy to get this wrong, but it doesn’t have to be poorly implemented: make configuration of a non-default backing implementation explicit, rather than relying on detecting what happens to be installed(*) in a “try/import…/except ImportError” cascade, and ensure the fascade library provides the same functional behavior regardless of which backing implementation is in use.

(*) Consumers aren’t in control of what transitive dependencies they pick up after all. So they could end up with requests as a transitive dep via some other direct dependency, but still want to use httpx as the backing implementation for this example library.

@abravalheri
Copy link
Contributor

As this topic afects the semantics of the whole Python packaging, I suppose the best place to discuss it is in https://discuss.python.org/c/packaging/14.

It is likely it will require someone to put forward a PEP and a proof-of-concept in terms of installer and build backend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Needs Discussion Issues where the implementation still needs to be discussed.
Projects
None yet
Development

No branches or pull requests