Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

Tensorflow requires manual install if going via conda #187

Closed
wants to merge 11 commits into from

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented Jul 3, 2023

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
Moves towards resolving conda-forge/cellfinder-core-feedstock#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

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:

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

@adamltyson
Copy link
Member

@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
Copy link
Collaborator Author

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
Copy link
Member

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 willGraham01 changed the title Tensorflow is an optional dependency Tensorflow requires manual install if going via conda Jul 4, 2023
@willGraham01
Copy link
Collaborator Author

willGraham01 commented Jul 4, 2023

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 willGraham01 removed the request for review from a team July 5, 2023 09:03
@willGraham01
Copy link
Collaborator Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants