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

Moving from Python3.8 to Python 3.11 #811

Merged
merged 29 commits into from
Dec 20, 2024
Merged

Moving from Python3.8 to Python 3.11 #811

merged 29 commits into from
Dec 20, 2024

Conversation

init-22
Copy link

@init-22 init-22 commented Nov 15, 2024 •
edited
Loading

Making the following major changes:

  • Python 3.11.10
  • Jax 0.4.35
  • Pytorch 2.5.1
  • Tensorflow 2.18

#805

@init-22 init-22 requested a review from a team as a code owner November 15, 2024 13:45
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 15, 2024 •
edited
Loading

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@priyakasimbeg priyakasimbeg changed the base branch from main to dev November 21, 2024 19:00
@priyakasimbeg priyakasimbeg changed the base branch from dev to main November 22, 2024 05:26
@priyakasimbeg
Copy link
Contributor

temporarily changed target branch to main to trigger regression tests.

@priyakasimbeg priyakasimbeg changed the base branch from main to dev November 22, 2024 05:28
@init-22
Copy link
Author

init-22 commented Nov 22, 2024

Some of the required package versions are only available in python3.11 and the tests are using python3.9 it seems, we might need to change that

@priyakasimbeg
Copy link
Contributor

Could you update the tests to use 3.11 in this PR?

init-22 added 14 commits November 30, 2024 13:07
@init-22
fix: changing the python versions in workflow to pass the tests
1bc2a7b
@init-22
fix: changing numpy compatible version
7a0fee3
@init-22
adding key_data to check the CI tests
7cdea16
@init-22
fix: updated packge of sacrebleu changed the way it used to work, hen…
7264c3f 
…ce using the corpus_bleu from the main package
@init-22
fix: temporarily commenting tfa
abbdc82
@init-22
fix: explicitly using mask kwarg to use MultiHeadDotProductAttention …
86029a7 
…and also using sacrebleu
@init-22
fix: using flax.core.pop instead of variables.pop, better way to upda…
aca45a2 
…te batch_stats
@init-22
fix: changing the traindiffs_tests branch to main again
2618c5e
@init-22
fix: unfreeze() in test_param_shapes expect FrozenDict also added fla…
8c90625 
…x.core.pop instead of variables.pop
@init-22
fix: formatting changes with yapf
1b587b7
@init-22
fix: running yapf again with 0.32, earlier using 0.43
c65d93e
@init-22
fix: running yapf again with 0.32, earlier using 0.43
3afd1df
@init-22
fix: latest versions of typing dont support Text instead str is recom…
6ff2010 
…mended
@init-22
fix: minor yapf
55bacbd
@priyakasimbeg
Copy link
Contributor

priyakasimbeg commented Dec 5, 2024 •
edited
Loading

Hey Isaac,
Could you paste the output of pip freeze in this PR as well of an environment setup with these upgrades?

Also I think we want to disable linting check in the lines that are failing the pylint tests.

@priyakasimbeg priyakasimbeg changed the base branch from dev to main December 5, 2024 18:24
@priyakasimbeg priyakasimbeg changed the base branch from main to dev December 5, 2024 18:25
@runame runame self-requested a review December 5, 2024 18:48
@priyakasimbeg priyakasimbeg changed the base branch from dev to main December 6, 2024 00:19
@priyakasimbeg
Copy link
Contributor

Running Containerized regression tests: https://github.com/mlcommons/algorithmic-efficiency/actions/runs/11934647960

@priyakasimbeg priyakasimbeg changed the base branch from main to python_upgrades December 6, 2024 00:28
@priyakasimbeg
Copy link
Contributor

@init-22 I'm going to merge this in to a new branch mlcommons/python_upgrades so that I can trigger the tests. Currently the containerized tests on our self-hosted runners can only run branches that are on this repo. Could you pull that branch and make follow up changes on there?

runame
runame reviewed Dec 6, 2024
View reviewed changes
Copy link
Contributor

@runame runame left a comment

Choose a reason for hiding this comment

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

What was the reason for switching to sacrebleu?

I haven't reviewed the Dockerfile and all the version updates, the rest LGTM.

algorithmic_efficiency/workloads/imagenet_resnet/imagenet_jax/randaugment.py Outdated Show resolved Hide resolved
algorithmic_efficiency/random_utils.py Show resolved Hide resolved
algorithmic_efficiency/workloads/wmt/wmt_jax/workload.py Outdated Show resolved Hide resolved
@init-22
Copy link
Author

init-22 commented Dec 7, 2024

@priyakasimbeg Please check the output of pip freeze

pip_freeze_python311.txt

runame
runame approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@runame runame left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks for porting the custom tf addons!

Maybe we can get rid of the sacrebleu dependency in a follow up PR (to avoid blocking this PR further).

algorithmic_efficiency/workloads/imagenet_resnet/imagenet_jax/randaugment.py Outdated Show resolved Hide resolved
@priyakasimbeg priyakasimbeg merged commit 9d1c957 into mlcommons:python_upgrades Dec 20, 2024
16 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@runame runame runame approved these changes

@priyakasimbeg priyakasimbeg priyakasimbeg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@init-22 @priyakasimbeg @runame