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

fix: Add dataset discovery tools to coffea.dataset_tools's __all__ and to docs #1144

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

rpsimeon34
Copy link
Contributor

The dataset discovery tools were not previously included in the dataset_tools sub package's __all__, resulting in them not appearing in the docs. This PR adds objects to __all__ and makes some edits so those objects are documented more clearly by Sphinx.

@lgray
Copy link
Collaborator

lgray commented Jul 31, 2024

including this package in __all__ will cause rucio to be a hard dependency of coffea, which will cause all testing to fail.

@rpsimeon34
Copy link
Contributor Author

I managed to get testing to pass by installing the dev and rucio dependency collections. For some reason I also had to manually pip install dogpile.cache (a dependency of rucio), but then pytest passed successfully. Are you opposed to me either
a. modifying the dev dependency collection to include rucio and dogpile.cache, or
b. creating a separate test dependency collection for testing?

@lgray
Copy link
Collaborator

lgray commented Aug 1, 2024

@rpsimeon34 can you use rucio-client as opposed to rucio. The latter is a much heftier/nasty dependency. It's completely fine with me if it goes in dev, so long as it works on all the versions of python we test against and all the OS's.

@ikrommyd
Copy link
Collaborator

ikrommyd commented Aug 1, 2024

@lgray I don't think rucio is available on windows. If so, it can't be a strict dependency.

@lgray
Copy link
Collaborator

lgray commented Aug 1, 2024

We're talking about the [dev] install extra, not the base dependencies. However, I think this doesn't actually get around the issue that importing coffea would cause a hard dependency on rucio or rucio-client.

I think the better fix here is to manually hint these files to sphinx, on top of the auto discovery.

@rpsimeon34
Copy link
Contributor Author

Apologies, I did mean rucio-client as the necessary package (I was referring to the [rucio] optional dependencies, and should have been clearer).

It looks to me like Windows isn't explicitly supported by rucio-clients (per this issue, which I don't think has been moved on at this point). So, I agree that addressing the dataset discovery files to Sphinx is probably safer than adding to __all__.

@lgray lgray merged commit 283cbca into CoffeaTeam:master Oct 16, 2024
1 of 3 checks passed
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