Tensorflow requires manual install if going via `conda`

brainglobe / cellfinder-core

Standalone cellfinder cell detection algorithm

https://brainglobe.info/documentation/cellfinder/index.html

BSD 3-Clause "New" or "Revised" License

19 stars 16 forks source link

Tensorflow requires manual install if going via `conda` #187

Closed willGraham01 closed 1 year ago

willGraham01 commented 1 year ago

Description

What is this PR

[x] Bug fix
[ ] Addition of a new feature
[ ] Other

Why is this PR needed? Moves towards resolving https://github.com/conda-forge/cellfinder-core-feedstock/issues/13.

What does this PR do? Updates the README to reflect the fact that conda installs will not fetch tensorflow - the users will have to manually install this themselves.

References

Resolves step 2, part 1 of https://github.com/brainglobe/BrainGlobe/issues/27
Moves towards resolution of https://github.com/conda-forge/cellfinder-core-feedstock/issues/13

How has this PR been tested?

Please explain how any new code has been tested, and how you have ensured that no existing functionality has changed.

Is this a breaking change?

For conda users, yes. But in terms of pip, everything is OK as before.

Does this PR require an update to the documentation?

README has been updated to reflect install instructions

Checklist:

[x] The code has been tested locally
[x] Tests have been added to cover all new functionality (unit & integration)
[x] The documentation has been updated to reflect any changes
[x] The code has been formatted with pre-commit

adamltyson commented 1 year ago

@willGraham01 can we not make tensorflow optional on conda-forge without doing this? Considering TF is always required for cellfinder, it seems bit backwards to make it optional for the pip package.

willGraham01 commented 1 year ago

can we not make tensorflow optional on conda-forge without doing this?

conda-forge doesn't have optional dependencies, but since conda-forge reads depenencies from the recipe rather than the pyproject.toml, we could give the conda package the same dependencies minus tensorflow. This (should) happily build and install given #186.

Since conda-forge fetches the source from PyPI, running pip check in the resulting environment will flag cellfinder-core as broken (which is probably the behaviour we want, given we know the conda install won't let users run anything). It does mean that we won't be able to have conda successfully verify the install in their CI checks, but I guess we can live with that?

If so, I'll keep the README changes in this PR here but revert tensorflow to required on pip, and open a PR on the cellfinder-core-feedstock.

adamltyson commented 1 year ago

Since conda-forge fetches the source from PyPI, running pip check in the resulting environment will flag cellfinder-core as broken (which is probably the behaviour we want, given we know the conda install won't let users run anything). It does mean that we won't be able to have conda successfully verify the install in their CI checks, but I guess we can live with that?

It's not great, but I think it's the best option for now.

willGraham01 commented 1 year ago

No problem - have kept the updated README text in anticipation of the conda-forge update, but reverted the dependency changes.

Once this goes in I'll upload another version (v0.4.2?) to PyPI with the #186 changes in it, and then hit the feedstock.

willGraham01 commented 1 year ago

Closing this in favour of the cleaner history in #191, and the discussion on the feedstock.