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

TensorFlow to PyTorch migration #189

Merged
merged 5 commits into from
Jun 4, 2024
Merged

TensorFlow to PyTorch migration #189

merged 5 commits into from
Jun 4, 2024

Conversation

IgorTatarnikov
Copy link
Member

@IgorTatarnikov IgorTatarnikov commented May 20, 2024

Before submitting a pull request (PR), please read the contributing guide.

Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
We're changing the backend for cellfinder to use PyTorch instead of Tensorflow.

What does this PR do?
Removes most mentions of TensorFlow as it relates to cellfinder and replaces them with the relevant PyTorch instruction.

I've made these changes with the understanding that cellfinder is the only package that currently uses TensorFlow so migrating to PyTorch allows us to not instruct users on setting it up. I didn't change the mentions in error-messages.md as those remain relevant for people using the older version.

References

Closes #183

How has this PR been tested?

Built locally.

Is this a breaking change?

No.

Checklist:

  • The code has been tested locally
  • The code has been formatted with pre-commit

@IgorTatarnikov
Copy link
Member Author

I had one question about these lines, after we remove TensorFlow are we still going to recommend a Python 3.10 dev environment? If so, what's our official reason?

@adamltyson @alessandrofelder

@IgorTatarnikov IgorTatarnikov marked this pull request as ready for review May 20, 2024 10:16
@IgorTatarnikov IgorTatarnikov requested a review from a team May 20, 2024 10:16
@adamltyson
Copy link
Member

Nope, once we move to torch, the plan is to update everything to recommend 3.11. Once all dependencies also support it, we can then move to 3.12.

tensorflow is/was the only thing keeping us on 3.10

@alessandrofelder
Copy link
Member

IIUC as we're following NEP 29, we will recommend 3.11 but still support 3.10 until April 2025

@adamltyson
Copy link
Member

Yeah we haven't updated supported versions while we've been stuck with 3.10 as the latest. I suggest once the torch version of cf is released, we go through and NEP29-ify everything.

Copy link
Member

@alessandrofelder alessandrofelder 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 to me - we should wait until brainglobe/cellfinder#418 (comment) is merged before merging this.

@adamltyson
Copy link
Member

Looks good to me - we should wait until brainglobe/cellfinder#418 (comment) is merged before merging this.

*is released.

@IgorTatarnikov
Copy link
Member Author

Build Sphinx Docs is failing due to:

(documentation/brainglobe-atlasapi/usage/atlas-details: line  131) broken    https://doi.org/10.1101/2021.05.04.442625 - HTTPConnectionPool(host='biorxiv.org', port=80): Max retries exceeded with url: /lookup/doi/10.1101/2021.05.04.442625 (Caused by ConnectTimeoutError(<urllib3.connection.HTTPConnection object at 0x7fd2980e6780>, 'Connection to biorxiv.org timed out. (connect timeout=30)'))

The link works when I try to access it via browser.

@adamltyson
Copy link
Member

I've re-run the build. I've sometimes noticed that biorxiv does this, but it works itself out

@IgorTatarnikov IgorTatarnikov mentioned this pull request Jun 3, 2024
7 tasks
@adamltyson adamltyson merged commit 1bc4237 into main Jun 4, 2024
3 checks passed
@adamltyson adamltyson deleted the tensorflow-to-torch branch June 4, 2024 13:41
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.

[Feature] Update cellfinder/brainmapper documentation to account for keras 3.0, torch migration
3 participants