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

Make some dependencies optional #2641

Merged
merged 12 commits into from
Nov 13, 2024
Merged

Make some dependencies optional #2641

merged 12 commits into from
Nov 13, 2024

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Nov 8, 2024

  • matplotlib
  • Bokeh
  • eventio
  • iminuit

@maxnoe
Copy link
Member Author

maxnoe commented Nov 8, 2024

People will get an OptionalDependencyMisisng error now for these:

In [3]: s = SimTelEventSource("dataset://gamma_prod5.simtel.zst")
---------------------------------------------------------------------------
OptionalDependencyMissing                 Traceback (most recent call last)
Cell In[3], line 1
----> 1 s = SimTelEventSource("dataset://gamma_prod5.simtel.zst")

File ~/Uni/CTA/ctapipe/src/ctapipe/io/simteleventsource.py:539, in SimTelEventSource.__init__(self, input_url, config, parent, **kwargs)
    537 super().__init__(input_url=input_url, config=config, parent=parent, **kwargs)
    538 if SimTelFile is None:
--> 539     raise OptionalDependencyMissing("eventio")
    541 self.file_ = SimTelFile(
    542     self.input_url.expanduser(),
    543     allowed_telescopes=self.allowed_tels,
    544     skip_calibration=self.skip_calibration_events,
    545     zcat=not self.back_seekable,
    546 )
    547 if self.back_seekable and self.is_stream:

OptionalDependencyMissing: 'eventio' is required for this functionality of ctapipe

@maxnoe
Copy link
Member Author

maxnoe commented Nov 8, 2024

And for using EventSource I added special handling for OptionalDependencyMissing:

ValueError: Could not find compatible EventSource for input_url: PosixPath('/home/maxnoe/.cache/ctapipe/cccta-dataserver.in2p3.fr/data/ctapipe-test-data/v1.1.0/gamma_prod5.simtel.zst')
in available EventSources: ['HDF5EventSource', 'PluginEventSource']
EventSources that could not be used due to missing optional dependencies:
	SimTelEventSource: eventio

This comment has been minimized.

@maxnoe maxnoe marked this pull request as ready for review November 8, 2024 15:00

This comment has been minimized.

@Tobychev
Copy link
Contributor

Tobychev commented Nov 8, 2024

Why strip out matplotlib? Is this to save time on the cluster setup? My first reaction is that this will make dev more annoying as I have to manually install it to make plots whenever I recreate the conda environment from the latest environment file.

Also, dropping ImPACT as a reco method available by default seems a bit drastic and something we're very likely to revert once we actually have a sensitivity curve for a tuned analysis.

Is it possible to make this stripped down version gated behind some special install instead of being the default case?

This comment has been minimized.

1 similar comment

This comment has been minimized.

@maxnoe
Copy link
Member Author

maxnoe commented Nov 11, 2024

Tests (including the new one only testing the strictly required dependencies) are now passing.

@kosack @Tobychev please have a look.

This comment has been minimized.

@maxnoe
Copy link
Member Author

maxnoe commented Nov 11, 2024

Why strip out matplotlib? Is this to save time on the cluster setup? My first reaction is that this will make dev more annoying as I have to manually install it to make plots whenever I recreate the conda environment from the latest environment file.

This was explicitly requested by the ACADA SAG group. And I agree, matplotlib is a heavy dependency that we shouldn't install in circumstances where only processing events is required.

Also, dropping ImPACT as a reco method available by default seems a bit drastic and something we're very likely to revert once we actually have a sensitivity curve for a tuned analysis.

I think you are confusing DataPipe and ctapipe requirements here, DataPipe is free to depend on ctapipe and iminuit to support ImPACT. Not all users of ctapipe want or need impact though.

Is it possible to make this stripped down version gated behind some special install instead of being the default case?

No, not at the moment, at least not for the PyPI package. You can only add optional dependencies, not provide a default that has more and an extra that strips it down.

conda-forge doesn't do optional dependencies at all, so what most larger projects do is provide a -base package that only contains the strictly required depdencies and the nominal package name that comes with all recommended depdendencies, e.g. matplotlib-base vs. matplotlib and astropy-base vs astropy (the latter one is planned for v7, not yet published).

@maxnoe
Copy link
Member Author

maxnoe commented Nov 11, 2024

Why strip out matplotlib? Is this to save time on the cluster setup? My first reaction is that this will make dev more annoying as I have to manually install it to make plots whenever I recreate the conda environment from the latest environment file.

Also note that the environment.yaml file hasn't changed. It still includes all optional dependencies. The dev extra has also been changed so that it includes all dev optional dependencies.

So

$ pip install -e '.[dev]'

will still give your everything

@maxnoe
Copy link
Member Author

maxnoe commented Nov 11, 2024

Astropy is currently preparing a PEP for making it possible to have more dependencies by default pip install astropy but being able to select a minimal installation pip install astropy[minimal], but the time scale is quite long I guess.

https://github.com/astrofrog/peps/pull/1/files

Copy link

Failed

  • 78.60% Coverage on New Code (is less than 80.00%)

Analysis Details

1 Issue

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 1 Code Smell

Coverage and Duplications

  • Coverage 78.60% Coverage (94.10% Estimated after merge)
  • Duplications 0.00% Duplicated Code (0.70% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

@maxnoe maxnoe merged commit 057c6e5 into main Nov 13, 2024
12 of 13 checks passed
@maxnoe maxnoe deleted the optional-deps branch November 13, 2024 14:52
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