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

Merged
merged 2 commits into from
Jan 6, 2025

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 ?

@astrofrog
Copy link
Member

@neutrinoceros - sounds good to me!

@neutrinoceros
Copy link
Contributor Author

@astrofrog done. Note that this is still blocked by astrofrog/mpl-scatter-density#43

@neutrinoceros
Copy link
Contributor Author

Actually, mpl-scatter-density 0.8 unblocked this (thanks !); undrafting now.

@neutrinoceros neutrinoceros marked this pull request as ready for review January 6, 2025 19:34
legacy: ipython==7.16.*
legacy: ipykernel==5.3.*
legacy: dill==0.2.*
legacy: xlrd==1.2.*
legacy: h5py==2.10.*
legacy: mpl-scatter-density==0.7.*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this part is now a natural follow up to #2521

@astrofrog astrofrog merged commit 40c58fb into glue-viz:main Jan 6, 2025
25 checks passed
@neutrinoceros neutrinoceros deleted the dep/drop_setuptools branch January 6, 2025 20:09
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