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

MAINT: add optional packages for tensorflow and tensorflow-cpu; update documentation #262

Merged
merged 4 commits into from
Jan 6, 2022

Conversation

metasyn
Copy link
Contributor

@metasyn metasyn commented Jan 5, 2022

Description

Fixes #261

Changes

poetry add --optional tensorflow==2.7.0
poetry add --optional tensorflow==2.7.0

Update documentation. I updated all pip install sites - but maybe only some need to be updated?

Copy link
Owner

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

Looks good, only issue is the pinned versions

Comment on lines -27 to -28
You will need to manually install TensorFlow; due to TensorFlow's packaging it is not a direct dependency of SciKeras.
You can do this by running:
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's worth leaving a comment in explaining why we recommend installing with extras. Something like:

SciKeras packages TensorFlow as an optional dependency because there are several flavors of TensorFlow available (tensorflow, tensorflow-cpu, etc.). pip install scikeras[tensorflow] is equivalent to pip install scikeras tensorflow and is offered just for convenience. You can also install just SciKeras with pip install scikeras, but you will need a version of tensorflow installed at runtime or SciKeras will throw an error when you try to import it.

@adriangb
Copy link
Owner

adriangb commented Jan 5, 2022

Thinking about this a bit more: do you see any advantage in making these optional dependencies scikeras[tensorflow] v.s. just editing the docs to read pip install scikeras tensorflow or pip install scikeras tensorflow-cpu @metasyn ? I'm open to either and I think both need the same explanation blurb in the docs, I'm just trying to think what might be less confusing for a user.

@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2022

Codecov Report

Merging #262 (6fd5a84) into master (57918d6) will decrease coverage by 0.13%.
The diff coverage is n/a.

❗ Current head 6fd5a84 differs from pull request most recent head cb422fd. Consider uploading reports for the commit cb422fd to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #262      +/-   ##
==========================================
- Coverage   98.27%   98.13%   -0.14%     
==========================================
  Files           7        7              
  Lines         752      752              
==========================================
- Hits          739      738       -1     
- Misses         13       14       +1     
Impacted Files Coverage Δ
scikeras/_saving_utils.py 96.73% <0.00%> (-1.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57918d6...cb422fd. Read the comment docs.

@metasyn
Copy link
Contributor Author

metasyn commented Jan 5, 2022

Thinking about this a bit more: do you see any advantage in making these optional dependencies scikeras[tensorflow] v.s. just editing the docs to read pip install scikeras tensorflow or pip install scikeras tensorflow-cpu @metasyn ? I'm open to either and I think both need the same explanation blurb in the docs, I'm just trying to think what might be less confusing for a user.

I guess adding it as an optional dependency makes it more obvious, in my opinion, in terms of what is going on. It reminds me of using coverage[toml] or dask[complete] - where its expected that w/o that, certain things wont work. It would be really nice if poetry somehow supported an "either or" kind of constraint or something.

I'm happy to change whatever you prefer though.

@adriangb
Copy link
Owner

adriangb commented Jan 5, 2022

I think this change works. I agree with your assessment of it being more "obvious" even if it's just thin syntactic sugar.

It would be really nice if poetry somehow supported an "either or" kind of constraint or something.

It's not necessarily a poetry issue (in fact, poetry kinda support this usage when installing a local package via dependency groups). Really this is a pip / package ecosystem issue: pypa/setuptools#1139

pyproject.toml Show resolved Hide resolved
@adriangb adriangb merged commit cbc7940 into adriangb:master Jan 6, 2022
@adriangb
Copy link
Owner

adriangb commented Jan 6, 2022

Thanks @metasyn !

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.

tensorflow should be a main dependency (instead of a dev dependency)
3 participants