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

requirements.txt #529

Merged
merged 34 commits into from
Dec 8, 2020
Merged

requirements.txt #529

merged 34 commits into from
Dec 8, 2020

Conversation

nikita-klsh
Copy link
Member

@nikita-klsh nikita-klsh commented Oct 15, 2020

This PR updates requirements.txt, adds the corresponding section to the documentation, and makes some imports optional.

@nikita-klsh nikita-klsh linked an issue Oct 15, 2020 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Oct 15, 2020

Codecov Report

Merging #529 (d4b4dcc) into master (cba76d5) will decrease coverage by 0.59%.
The diff coverage is 20.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #529      +/-   ##
==========================================
- Coverage   41.64%   41.05%   -0.60%     
==========================================
  Files         152      156       +4     
  Lines       15304    15555     +251     
==========================================
+ Hits         6374     6386      +12     
- Misses       8930     9169     +239     
Impacted Files Coverage Δ
batchflow/batch_image.py 23.65% <ø> (-6.61%) ⬇️
batchflow/components.py 77.91% <0.00%> (ø)
batchflow/models/tf/__init__.py 97.56% <ø> (ø)
batchflow/models/torch/base.py 10.59% <0.00%> (-0.16%) ⬇️
batchflow/models/torch/callbacks/__init__.py 0.00% <0.00%> (ø)
batchflow/models/torch/callbacks/base.py 0.00% <0.00%> (ø)
batchflow/models/torch/callbacks/plateau.py 0.00% <0.00%> (ø)
batchflow/models/torch/layers/conv_block.py 21.39% <ø> (+0.11%) ⬆️
batchflow/opensets/imagenette.py 0.00% <0.00%> (ø)
batchflow/research/__init__.py 100.00% <ø> (ø)
... and 21 more

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 9c96747...d4b4dcc. Read the comment docs.

@nikita-klsh nikita-klsh changed the title requirements requirements.txt Oct 15, 2020
@roman-kh
Copy link
Member

Are all of these are needed when you import batchflow?
For instance, torch is required only when you import something from batchflow.models.torch.
Would be great to add documentation which clearly describes which packages are needed for which batchflow modules.

requirements.txt Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
requirements.txt Outdated
requests
scikit-image
scikit-learn
scipy
Copy link
Member

Choose a reason for hiding this comment

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

is it really needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's used in the images_batch.py.
All the functions from scipy.ndimage are bounded to the ImagesBatch methods via @add_methods decorator.

Moreover, skimage.transform.resize method added through the same decorator, which implies additional dependency. The same resize method already explicitly implemented in the class. So I suggest removing one added via decorator(the same for np.pad method)

P.S. I do not succeed in launching any of these methods and did not find examples how to.

batchflow/research/__init__.py Outdated Show resolved Hide resolved
batchflow/models/torch/__init__.py Outdated Show resolved Hide resolved
batchflow/models/tf/__init__.py Outdated Show resolved Hide resolved
.. note::

This requires torch
.. automodule:: batchflow.models.torch
Copy link
Member

Choose a reason for hiding this comment

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

automodule is not a good idea as it will include everything public in an inconvenient order

Copy link
Member Author

Choose a reason for hiding this comment

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

According to the docs it will insert only the docstring of the object by default

docs/index.rst Outdated
* numba
* pandas
* psutil
* scipy
Copy link
Member

Choose a reason for hiding this comment

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

Where do we need scipy? in samplers only? Import it only when really needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Scipy is also needed for instance segmentation utility function get_components .
The metrics module are being imported in pipeline.py.
We can exclude utils functions from metrics.__init__.py and it will be fine. Or make the scipy import optional there.

docs/index.rst Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
@roman-kh
Copy link
Member

roman-kh commented Nov 2, 2020

Please, resolve the conflict and update reqs in accordance with latest pull requests

@roman-kh roman-kh linked an issue Dec 2, 2020 that may be closed by this pull request
@roman-kh roman-kh merged commit fce8e7e into master Dec 8, 2020
@roman-kh roman-kh deleted the requirements branch December 8, 2020 08:39
@roman-kh roman-kh linked an issue Mar 19, 2021 that may be closed by this pull request
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.

Minimize requirements Update requirements Check install requirements
2 participants