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

Improve info tool dependencies option #2303

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

HealthyPear
Copy link
Member

While porting this for SWGO software I modified the code to read the dependencies at runtime from setup.cfg (like we do already for the Sphinx documentation configuration).

While doing this I noticed that the code using importlib.import_module sometimes gives rise to a ModuleNotFoundErrorexception, so I added it to the first try/except block to fallback to importlib.metadata.version and only if that also fails we give up.

I also used pytest-console-scripts to create 1 test for each flag (but see this issue I opened there, even though I tested each of them separately).

@HealthyPear HealthyPear force-pushed the improve_info_tool_dependencies branch from b16e27c to f2e4c43 Compare March 31, 2023 14:35
@HealthyPear
Copy link
Member Author

It seems that the online tests fail to find the option part of setup.cfg or the file itself.

Locally it works as expected.

@maxnoe
Copy link
Member

maxnoe commented Mar 31, 2023

The tests are run on the installed ctapipe, which doesn't have a setup.cfg

@HealthyPear
Copy link
Member Author

The tests are run on the installed ctapipe, which doesn't have a setup.cfg

Dammit...

And I guess this
pypa/packaging-problems#215
kills the possibility to use e.g importlib.metadata to access this information from user-like installations, am I right?

Can't we run the tests on the development version and in a separate job just test that the user installation simply doesn't crash? The code to test is the same.

@HealthyPear
Copy link
Member Author

HealthyPear commented Mar 31, 2023

It seems that there is at least importlib.metadata.requires

@kosack
Copy link
Contributor

kosack commented Apr 5, 2023

You can't read setup.cfg in ctapipe-info, since that file is outside of the package. But perhaps there is a way to do something similar to how we do the version string: there a file gets generated during the install process that becomes part of the package. So in the install, you could generate a "dependencies" list and then that would be available in the package.

The main issue I have with that is that ctapipe-info should also list optional dependencies, since that info is very useful to know for debugging. But I suppose you could parse options.extras_require , but then you might get lots of random things about the documentation building, and not generally optional run-time packages like bokeh (which now is required, but should be optional)

@maxnoe
Copy link
Member

maxnoe commented Apr 5, 2023

In general, any functionality like this should use the new importlib.metadata (importlib_metadata before 3.9) module prodiving such information independently of the setup.cfg / pyproject.toml or whatever packaging system is used.

@HealthyPear
Copy link
Member Author

I used importlib.metadata to extract the information about the complete set of packages and I added the splitting of optional packages based on the extras provided by the package metadata.

The internal approach is somewhat more brute-force than I wished: it appears that from Python3.10 that library allows using JSON to explore the metadata and simplifies it a bit, but since we test from 3.9 I had to keep things compatible with the older API.

The unit-tests I added based on pytest-console-scripts now work as intended for some reason.

@HealthyPear HealthyPear requested review from kosack and maxnoe May 8, 2023 10:28
@HealthyPear
Copy link
Member Author

On a side note: shouldn't ctapipe-info --resources also show the existing cached resources (i.e. ~/.cache/ctapipe/....)?

@kosack
Copy link
Contributor

kosack commented Sep 6, 2023

On a side note: shouldn't ctapipe-info --resources also show the existing cached resources (i.e. ~/.cache/ctapipe/....)?

It does:

> ctapipe-info --resources  
*** ctapipe resources ***

CTAPIPE_SVC_PATH: (directories where resources are searched)
	 no path is set

RESOURCE NAME                  : LOCATION
----------------------------------------------------------------------
ASTRICam.camgeom.fits.gz       : ~/.cache/ctapipe/cccta-dataser
CHEC.camgeom.fits.gz           : ~/.cache/ctapipe/cccta-dataser
DigiCam.camgeom.fits.gz        : ~/.cache/ctapipe/cccta-dataser
FACT.camgeom.fits.gz           : ~/.cache/ctapipe/cccta-dataser

for package in all_dependencies:
if "extra" in package:
for extra in extras:
if extra in package:
Copy link
Member

Choose a reason for hiding this comment

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

there is the same if twice here

version = get_module_version(name)
print(f"{name:>20s} -- {version}")
meta = metadata("ctapipe")
extras = [v for k, v in meta.items() if k == "Provides-Extra"]
Copy link
Member

Choose a reason for hiding this comment

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

This can be:

In [10]: meta.get_all("Provides-Extra")
Out[10]: ['all', 'dev', 'docs', 'tests']

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.

3 participants