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

[sharktank] Extend to test with Python 3.12 #693

Merged
merged 4 commits into from
Dec 13, 2024

Conversation

marbre
Copy link
Collaborator

@marbre marbre commented Dec 13, 2024

Progress on #357

Extends to test with Python 3.12 and newer torch versions on Linux.

For torch.compile support on Python 3.12, torch ge 2.4.0 is required. Testing with torch 2.3.0 is therefore skipped when running Python 3.12. To limit the number of jobs, tests are currently only expanded for Linux, whereas Windows (and unpinning NumPy) should be handled in a follow up.

The error logged for testExportNondecomposed when failing with torch 2.5.1 slows down the CI by more than 20 minutes, therefore the test is now skipped instead of marked xfail.

@marbre marbre force-pushed the sharktank-extend-ci branch 5 times, most recently from 6e80f5d to e52a7d1 Compare December 13, 2024 13:40
@marbre marbre force-pushed the sharktank-extend-ci branch from e52a7d1 to a22b1ac Compare December 13, 2024 13:51
@marbre marbre requested a review from ScottTodd December 13, 2024 14:38
@marbre marbre marked this pull request as ready for review December 13, 2024 14:38
Copy link
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

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

Nice! I made some small edits to the PR description and left some suggestions for abbreviating the job names

@@ -26,8 +26,17 @@ jobs:
name: "Unit Tests and Type Checking"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename to just "Unit Tests" or "Test" so more of the matrix shows up in the UI

image

Could also qualify "torch {{ matrix.torch-version }}"

Here's a mock-up to show where the text gets cut off:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I renamed to "Unit Tests" for now but we probably want to harmonize this more across different workflows in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Nice. Definitely better now. Could still include "torch" next to the torch version but it might be obvious enough to some people.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed to "Unit Tests :: ${{ matrix.os }}, ${{ matrix.python-version }}, ${{ matrix.torch-version }}" as for Windows it was (windows-2022, 3.11, 2.3.0) and now have the same order for all jobs. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, the ordering helps too. I think having two versions right next to each other can be confusing unless you are very familiar with Python and PyTorch versions. I personally have to unpack 3.11, 2.4.1 and think for a second, compared to 3.11, torch 2.4.1. We don't have many characters to spare in the UI though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Save another character but we should aim to harmonize the naming in a follow up.

.github/workflows/ci-sharktank.yml Outdated Show resolved Hide resolved
@marbre marbre enabled auto-merge (squash) December 13, 2024 17:42
@marbre marbre disabled auto-merge December 13, 2024 17:42
@marbre marbre force-pushed the sharktank-extend-ci branch from 503a565 to 402e2cc Compare December 13, 2024 18:08
@marbre marbre force-pushed the sharktank-extend-ci branch from 402e2cc to d435d97 Compare December 13, 2024 18:12
@marbre marbre merged commit 332baa3 into nod-ai:main Dec 13, 2024
12 checks passed
@marbre marbre deleted the sharktank-extend-ci branch December 13, 2024 18:36
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.

2 participants