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

Use extras_require in setup.py for optional dependencies #371

Merged
merged 7 commits into from
Sep 8, 2020

Conversation

mivade
Copy link
Contributor

@mivade mivade commented Aug 5, 2020

Include requests and aiohttp as both appear to be required for HTTP support now.

This relates to comments in #366

Are there any other optional dependencies we should add here? Once those are added I would also update the documentation to include hints about this, so I'm marking this as a draft PR for now.

@mivade mivade changed the title Add extras_require for http Use extras_require in setup.py for optional dependencies Aug 5, 2020
@martindurant
Copy link
Member

Yes, quite a few! Any of the items in registry that are imported from within the fsspec package and have error messages, e.g., distributed for dask, paramiko for ssh, pyarrow for hdfs.

@mivade
Copy link
Contributor Author

mivade commented Aug 5, 2020

Thanks @martindurant, I'll take a look there.

setup.py Outdated
@@ -32,5 +32,19 @@
packages=["fsspec", "fsspec.implementations"],
python_requires=">3.6",
install_requires=open("requirements.txt").read().strip().split("\n"),
extras_require={
"aws": ["s3fs"],
Copy link
Member

Choose a reason for hiding this comment

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

I would probably use the same names as the protocols, so "s3" for this one and "gcs" instead of "google"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure what the best approach was there. I think matching it is fine even if that means there are some redundancies.

Not all included filesystems are usable by default without installing extra
dependencies. For example to be able to access data in S3::

pip install fsspec[s3]
Copy link
Member

Choose a reason for hiding this comment

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

Missing s3 in the list :)

Copy link
Member

Choose a reason for hiding this comment

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

Please also check that this works - because s3fs depends on fsspec, fsspec[s3] would depend on itself.

@martindurant
Copy link
Member

This PR is still marked as draft. Ready to go?
I have not personally checked that something like pip install fsspec[s3] would do the right thing.

@mivade
Copy link
Contributor Author

mivade commented Aug 7, 2020

Not yet. I still want to add automated testing that the extras work. I'll mark it as ready for review once I've done so.

@martindurant
Copy link
Member

ping @mivade , do you think you'll have time to finish this PR off?

@mivade
Copy link
Contributor Author

mivade commented Sep 1, 2020

Yeah, I should be able to get to the testing later this week. Sorry for the delay!

@mivade mivade marked this pull request as ready for review September 4, 2020 20:42
@mivade
Copy link
Contributor Author

mivade commented Sep 4, 2020

@martindurant I've added some pretty basic testing here. I took the existing S3 and GCSFS tox environments and removed the conda dependencies installed there to instead use the extras= line. This tells tox to install the package with the provided extras (e.g., something like pip install .[gcs]). Of course this doesn't test every single extra defined in this PR; doing so would require some considerable rework of how the test environments are setup.

@martindurant
Copy link
Member

Looks good!

@martindurant martindurant merged commit b54081d into fsspec:master Sep 8, 2020
@mivade mivade deleted the add-extras branch September 8, 2020 15:08
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