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

fix(artifacts): remove suspect characters when directory creation fails #6094

Merged
merged 10 commits into from
Aug 18, 2023

Conversation

moredatarequired
Copy link
Contributor

@moredatarequired moredatarequired commented Aug 17, 2023

Fixes

Description

Supplants #5022 : I still think that's the "right" approach, but this is safer (just more complicated).

  • when creating directories for artifact download, try to use the previous ("system preferred") path, but if that fails due to invalid characters keep trying alternative directories until directory creation succeeds, and use that
  • when retrieving the "download root" for an artifact, first look for either of the places we expect, and return the one that exists; if neither exist then return the preferred system path

For 99.99% of users (literally, I think), this PR should have no impact. For the very small number of users who are on a non-Windows system but downloading their artifact files to an NTFS or FAT file system, this should prevent them from running into OSError: [Errno 22] Invalid argument: './artifacts/model:v55' through no fault of their own.

🤖 Generated by Copilot at b92eb97

Improved the compatibility and robustness of the Artifact class and related file operations across different systems and filesystems. Refactored and cleaned up some code in artifact.py, filesystem.py, and filenames.py.

Testing

Whew, still needs tests for things other than the happy path.

Checklist

  • Include reference to internal ticket "Fixes WB-NNNNN" and/or GitHub issue "Fixes #NNNN" (if applicable)
  • Ensure PR title compliance with the conventional commits standards

🤖 Generated by Copilot at b92eb97

To make artifacts work on all systems
We removed some unused code and symbols
We used filesystem
To handle paths with wisdom
And cleaned up filenames from the lint problems

@github-actions github-actions bot added cc-fix and removed cc-fix labels Aug 17, 2023
@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Merging #6094 (35307a6) into main (ffe22c9) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6094      +/-   ##
==========================================
- Coverage   77.90%   77.85%   -0.06%     
==========================================
  Files         379      379              
  Lines       44294    44321      +27     
==========================================
- Hits        34508    34504       -4     
- Misses       9736     9767      +31     
  Partials       50       50              
Flag Coverage Δ
unittest 81.52% <100.00%> (-0.07%) ⬇️

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

Files Changed Coverage Δ
wandb/sdk/lib/filenames.py 100.00% <ø> (ø)
wandb/sdk/artifacts/artifact.py 91.23% <100.00%> (-0.04%) ⬇️
wandb/sdk/lib/filesystem.py 97.88% <100.00%> (+0.56%) ⬆️

... and 14 files with indirect coverage changes

@dmitryduev dmitryduev added this to the sdk-2023-09.1 milestone Aug 17, 2023
wandb/sdk/lib/filesystem.py Outdated Show resolved Hide resolved
wandb/sdk/lib/filesystem.py Outdated Show resolved Hide resolved
wandb/sdk/lib/filesystem.py Show resolved Hide resolved
@moredatarequired moredatarequired merged commit 1771a8a into main Aug 18, 2023
65 checks passed
@moredatarequired moredatarequired deleted the try-alternate-directory-names branch August 18, 2023 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants