adriangb / scikeras

Scikit-Learn API wrapper for Keras.
https://www.adriangb.com/scikeras/
MIT License
239 stars 47 forks source link

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

Closed metasyn closed 2 years ago

metasyn commented 2 years ago

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?

adriangb commented 2 years ago

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 commented 2 years ago

Codecov Report

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

:exclamation: 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%) :arrow_down:

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 commented 2 years ago

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 commented 2 years ago

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: https://github.com/pypa/setuptools/issues/1139

adriangb commented 2 years ago

Thanks @metasyn !