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.

Sign up for GitHub

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

Jump to bottom

[MNT] Swapped tensorflow and pytorch to install only CPU version #2416

Merged
merged 5 commits into from
Dec 21, 2024
Merged

[MNT] Swapped tensorflow and pytorch to install only CPU version #2416

merged 5 commits into from
Dec 21, 2024

Conversation

chrisholder
Copy link
Contributor

@chrisholder chrisholder commented Nov 28, 2024 •
edited
Loading

Reference Issues/PRs

#2415

What does this implement/fix? Explain your changes.

This PR adds a custom github action when installing all_extras on ubuntu. The custom action installs tensorflow-cpu and pytorch cpu instead of the "normal" version which comes with all the GPU code. By installing the CPU version it saves considerable install time but in addition saves a considerable amount of RAM usage during the test running (as the package sizes are dramatically smaller). This should help to prevent issues causing the CI to crash due to exceeding memory limits.

Does your contribution introduce a new dependency? If yes, which one?

Any other comments?

PR checklist

For all contributions
  • I've added myself to the list of contributors. Alternatively, you can use the @all-contributors bot to do this for you.
  • The PR title starts with either [ENH], [MNT], [DOC], [BUG], [REF], [DEP] or [GOV] indicating whether the PR topic is related to enhancement, maintenance, documentation, bugs, refactoring, deprecation or governance.
For new estimators and functions
  • I've added the estimator to the online API documentation.
  • (OPTIONAL) I've added myself as a __maintainer__ at the top of relevant files and want to be contacted regarding its maintenance. Unmaintained files may be removed. This is for the full file, and you should not add yourself if you are just making minor changes or do not want to help maintain its contents.
For developers with write access
  • (OPTIONAL) I've updated aeon's CODEOWNERS to receive notifications about future changes to these files.

@chrisholder chrisholder requested a review from a team as a code owner November 28, 2024 17:05
@aeon-actions-bot aeon-actions-bot bot added the maintenance Continuous integration, unit testing & package distribution label Nov 28, 2024
@aeon-actions-bot
Copy link
Contributor

Thank you for contributing to aeon

I have added the following labels to this PR based on the title: [ $\color{#EC843A}{\textsf{maintenance}}$ ].

The Checks tab will show the status of our automated tests. You can click on individual test runs in the tab or "Details" in the panel below to see more information if there is a failure.

If our pre-commit code quality check fails, any trivial fixes will automatically be pushed to your PR unless it is a draft.

Don't hesitate to ask questions on the aeon Slack channel if you have any.

PR CI actions

These checkboxes will add labels to enable/disable CI functionality for this PR. This may not take effect immediately, and a new commit may be required to run the new configuration.

  • Run pre-commit checks for all files
  • Run mypy typecheck tests
  • Run all pytest tests and configurations
  • Run all notebook example tests
  • Run numba-disabled codecov tests
  • Stop automatic pre-commit fixes (always disabled for drafts)
  • Disable numba cache loading
  • Push an empty commit to re-run CI checks

@TonyBagnall TonyBagnall added full pytest actions Run the full pytest suite on a PR no numba cache Disable numba cache loading on a PR labels Nov 28, 2024
@chrisholder
Copy link
Contributor Author

Do not merge this yet need to get rid of the additional periodic test which I turned on just for this branch.

MatthewMiddlehurst
MatthewMiddlehurst reviewed Dec 2, 2024
View reviewed changes
Copy link
Member

@MatthewMiddlehurst MatthewMiddlehurst left a comment

Choose a reason for hiding this comment

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

  1. why only linux?
  2. doesn't this still install tensorflow and torch through all_extras?
  3. wouldnt we want to still install the gpu versions somewhere in the testing workflows?

@chrisholder
Copy link
Contributor Author

chrisholder commented Dec 2, 2024 •
edited
Loading

  1. why only linux?
  2. doesn't this still install tensorflow and torch through all_extras?
  3. wouldnt we want to still install the gpu versions somewhere in the testing workflows?

why only linux?
By default windows and macos doesn't install the gpu version so you only need to opt out on linux. There docs say:

Windows:
Caution: TensorFlow 2.10 was the last TensorFlow release that supported GPU on native-Windows. Starting with TensorFlow 2.11, you will need to install [TensorFlow in WSL2](https://tensorflow.org/install/pip#windows-%5Bwsl2%5D), or install tensorflow-cpu and, optionally, try the [TensorFlow-DirectML-Plugin](https://github.com/microsoft/tensorflow-directml-plugin#tensorflow-directml-plugin-)

Macos:
Currently there is no official GPU support for running TensorFlow on MacOS. The following instructions are for running on CPU.

The same is true for Torch macos doesn't support GPU, and windows you have to specify additional parameters to get the GPU version (which we don't currently do).

doesn't this still install tensorflow and torch through all_extras?
I believe if you install the dependency (i.e. tensorflow/torch) on a specific version and then run pip install .[all_extras] it won't reinstall them as they already are installed (and pip won't try change the version). While tensorflow-cpu has a different name it resolves to tensorflow so pip doesn't know the difference. I checked the logs here: https://github.com/aeon-toolkit/aeon/actions/runs/12073657794/job/33670204314?pr=2416#step:5:346 and it seems that it doesn't try reinstall the dependency.

wouldnt we want to still install the gpu versions somewhere in the testing workflows?
I don't think so? The code that we run in the CI will be the exact same. Tensorflow-cpu is literally just the same code but all the extra gpu "stuff" is excluded from the build (which we never ran for any OS in the CI). Unless we're able to run the CI with a GPU then I don't think there is a point.

@MatthewMiddlehurst
Copy link
Member

MatthewMiddlehurst commented Dec 4, 2024 •
edited
Loading

ok, let me know when its ready

@chrisholder
Copy link
Contributor Author

Ready for review @MatthewMiddlehurst

MatthewMiddlehurst
MatthewMiddlehurst approved these changes Dec 19, 2024
View reviewed changes
Copy link
Member

@MatthewMiddlehurst MatthewMiddlehurst left a comment

Choose a reason for hiding this comment

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

lgtm

@chrisholder chrisholder merged commit a4b8b3f into main Dec 21, 2024
18 checks passed
@chrisholder chrisholder deleted the periodic-test-fix branch December 21, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MatthewMiddlehurst MatthewMiddlehurst MatthewMiddlehurst approved these changes

Assignees
No one assigned
Labels
full pytest actions Run the full pytest suite on a PR maintenance Continuous integration, unit testing & package distribution no numba cache Disable numba cache loading on a PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chrisholder @MatthewMiddlehurst @TonyBagnall