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

Update torchx for fsspec 2023.10 #784

Merged
merged 1 commit into from
Nov 2, 2023
Merged

Update torchx for fsspec 2023.10 #784

merged 1 commit into from
Nov 2, 2023

Conversation

alanhdu
Copy link
Contributor

@alanhdu alanhdu commented Oct 30, 2023

This updates the code to the latest version of fsspec, updating the test to work around fsspec/filesystem_spec#1394.

I have written the test so that it should actually be compatible with both the old and new versions of fsspec, but for testing purposes updated the requirements.txt as well.

Supersedes #783 (so it can be merged separately from the internal diff).

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 30, 2023
@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Merging #784 (e232ceb) into main (1fe0607) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #784   +/-   ##
=======================================
  Coverage   92.88%   92.88%           
=======================================
  Files          96       96           
  Lines        6099     6099           
=======================================
  Hits         5665     5665           
  Misses        434      434           
Flag Coverage Δ
unittests 92.88% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kiukchung
Copy link
Collaborator

LGTM. Any idea why the unittest: torchx/schedulers/test/local_scheduler_test.py::LocalDirectorySchedulerTest::test_no_orphan_process_function is failing?

@alanhdu
Copy link
Contributor Author

alanhdu commented Nov 2, 2023

I have no idea -- when I tried to run it locally, everything seemed to pass. It looks like when someone also retried the github job and it's passing now?

@kiukchung
Copy link
Collaborator

I have no idea -- when I tried to run it locally, everything seemed to pass. It looks like when someone also retried the github job and it's passing now?

@kurman (can you help taking a look at the error logs for kfp?) I retried KFP Integration Tests to see if it was something flaky causing the failure, but the retry failed as well.

@kiukchung kiukchung merged commit fb819df into pytorch:main Nov 2, 2023
23 of 24 checks passed
@alanhdu alanhdu deleted the fsspec branch November 2, 2023 15:45
@kiukchung
Copy link
Collaborator

merged since KFP Integ tests seem broken in other PRs as well (#781, #784). Since we just bumped to 0.6.0dev, we can forward fix the test if these PRs continue breaking KFP tests in main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants