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

DEP: drop runtime dependency on setuptools #2511

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

neutrinoceros
Copy link
Contributor

Description

This is a follow up to #2365
At the moment this breaks tests because mpl-scatter-density has an undeclared runtime dependency on setuptools
(addressed at astrofrog/mpl-scatter-density#43)

I also note that the following comment

glue/glue/main.py

Lines 75 to 87 in e9df453

# We don't use item.load() because that then checks requirements of all
# the imported packages, which can lead to errors like this one that
# don't really matter:
#
# Exception: (pytest 2.6.0 (/Users/tom/miniconda3/envs/py27/lib/python2.7/site-packages),
# Requirement.parse('pytest>=2.8'), set(['astropy']))
#
# Just to be clear, this kind of error does indicate that there is an
# old version of a package in the environment, but this can confuse
# users as importing astropy directly would work (as setuptools then
# doesn't do a stringent test of dependency versions). Often this kind
# of error can occur if there is a conda version of a package and an
# older pip version.

which was introduced in #1487, has been irrelevant since #2365 (at least in part) since it mentions behavior details that were specific to setuptools. Should this comment be removed entierely or just rephrased ?

@neutrinoceros neutrinoceros changed the title DEP: drop runtime dependency on setuptools DEP: drop runtime dependency on setuptools Aug 30, 2024
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Just a small comment below. Otherwise, regarding the comment about not using item.load(), are you sure that these kind of errors should not happen anymore? If not, then in addition to removing the comment we should then actually use item.load() below in the try...except?


import setuptools
logger.info("Loading external plugins using "
"setuptools=={0}".format(setuptools.__version__))
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to keep 'Loading external plugins' - just don't give the setuptools version

@neutrinoceros
Copy link
Contributor Author

are you sure that these kind of errors should not happen anymore?

I don't know if there's a test to exercise this condition, but I tried replacing module = import_module(item.module) with module = item.load() and found it broke some tests for a different reason: EntryPoint.load doesn't always return a module, which is actually documented in this function's docstring:

        """Load the entry point from its definition. If only a module
        is indicated by the value, return that module. Otherwise,
        return the named object.
        """

so it stills seems simpler/safer to use import_module here, but I think we should update the comment to reflect the real reason. Should I do it 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.

2 participants