Skip to content

Removed most s3 bucket based tests (replaced with UC Volumes) #3869

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
Merged
merged 13 commits into from
Jun 6, 2025
Merged

Removed most s3 bucket based tests (replaced with UC Volumes) #3869

merged 13 commits into from
Jun 6, 2025

Conversation

rithwik-db
Copy link
Contributor

What does this PR do?

Essentially, all usages of s3_bucket has been removed outside of test_object_store.py and test_s3_object_store.py since we have the ephemeral and read_only paths in UC Volumes now.

rithwik-db
rithwik-db commented May 30, 2025
View reviewed changes
rithwik-db
rithwik-db commented May 31, 2025
View reviewed changes
@rithwik-db rithwik-db requested a review from a team as a code owner June 2, 2025 20:58
dakinggg
dakinggg reviewed Jun 3, 2025
View reviewed changes
rithwik-db added 3 commits June 4, 2025 16:37
@rithwik-db
This is a combination of 8 commits.
304cb8e 
removed s3 bucket usage

formatted

moved to read_only

formatted

hopefully works

this should work

hopefully cpu passes

moved tests to gpu since they don't need cpu
@rithwik-db
hopefully this works
600420e
@rithwik-db
formatted
9dc42bc
@rithwik-db
Copy link
Contributor Author

@dakinggg I keep getting this error on daily tests even though it seems to work perfectly on my remote instance. Just wanted to check if you've encountered this issue before...

@rithwik-db rithwik-db requested a review from dakinggg June 5, 2025 02:36
@dakinggg
Copy link
Contributor

dakinggg commented Jun 5, 2025

Ah I think its because dist is initialized with gpu (i.e. nccl) rather than cpu (i.e gloo) when running the gpu tests. its probably not worth adjusting test set up to support this, and just let the test run on gpu instead of cpu

rithwik-db
rithwik-db commented Jun 5, 2025
View reviewed changes
rithwik-db
rithwik-db commented Jun 6, 2025
View reviewed changes
@dakinggg
Copy link
Contributor

dakinggg commented Jun 6, 2025

It actually passed on 2.7 🤔

Screenshot 2025-06-05 at 7 22 21 PM

@dakinggg
Copy link
Contributor

dakinggg commented Jun 6, 2025

I'm ok just adjusting the tolerance for 2.6 if thats sufficient for the test to pass. Not super critical to keep exact numerical determinism across torch versions - and in fact on GPU I'd guess this might not be possible.

@rithwik-db
Copy link
Contributor Author

@dakinggg the daily tests pass when I use a separate checkpoint for 2.6 vs 2.7

@dakinggg
Copy link
Contributor

dakinggg commented Jun 6, 2025

ok, good enough for me. deterministically resuming a run from an older version of torch is a serious edge case.

dakinggg
dakinggg approved these changes Jun 6, 2025
View reviewed changes
@rithwik-db
Copy link
Contributor Author

Reran daily tests to make sure with latest fixes: https://github.com/mosaicml/composer/actions/runs/15500553934

@rithwik-db rithwik-db merged commit 96db24c into main Jun 6, 2025
28 checks passed
@rithwik-db rithwik-db deleted the use-volumes branch June 6, 2025 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@dakinggg dakinggg dakinggg approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rithwik-db @dakinggg