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 download_zip #2787

Merged
merged 6 commits into from
Jun 14, 2022
Merged

fix download_zip #2787

merged 6 commits into from
Jun 14, 2022

Conversation

akaszynski
Copy link
Member

@akaszynski akaszynski commented Jun 14, 2022

PR #2727 introduced a breaking change to our downloads module CI/CD is failing. It should have bumped the cache version in that PR and discovered the flaw with the PR.

Turns out there's an edge case that #2787 didn't consider with a fresh cache: a zip file (without extension) that's the same name as a directory. For example, we have Ensight an Ensight.zip. If the Ensight directory already exists locally, our Ensight.zip won't be downloaded and PyVista will look in the Ensight directory. However, since it's a different directory altogether, the downloads module won't find the file it's looking for and will fail outright.

The fix for this is to simply keep the "zip" in the directory name. It will look a little strange, but this is good in two ways:

  • Matches the directory structure of vtk-data better. Archives will simply appear as directories.
  • CI/CD works again.

Recommend immediate merge to get this CI working again.

adeak
adeak previously approved these changes Jun 14, 2022
Copy link
Member

@adeak adeak left a comment

Choose a reason for hiding this comment

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

Go for it.

@akaszynski
Copy link
Member Author

Go for it.

I'll merge if passing.

@github-actions github-actions bot added the bug Uh-oh! Something isn't working as expected. label Jun 14, 2022
@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

Merging #2787 (f10ab81) into main (91a562f) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2787      +/-   ##
==========================================
- Coverage   93.96%   93.90%   -0.06%     
==========================================
  Files          76       76              
  Lines       16292    16292              
==========================================
- Hits        15308    15299       -9     
- Misses        984      993       +9     

@akaszynski
Copy link
Member Author

@adeak, this needs another look over. Turns out it's not as easy as a cache bump.

@adeak adeak dismissed their stale review June 14, 2022 08:31

Tougher nut than expected

@akaszynski
Copy link
Member Author

Turns out files are hard.

Had to handle another edge case where the retriever returns the exact same path as the requested filename. This edge case only occurs in testing, but I figure if someone wanted to implement a custom retriever for whatever reason in the future we should handle that as well rather than modifying the test.

@akaszynski
Copy link
Member Author

Ok, ready to review. There was one final fix needed in bdd3248: our example was assuming paths.

Copy link
Member

@adeak adeak left a comment

Choose a reason for hiding this comment

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

I've pushed a few cosmetic changes (some unrelated to the PR), please check. Otherwise this LGTM, thanks

@akaszynski akaszynski changed the title bump cache version fix download_zip Jun 14, 2022
@akaszynski
Copy link
Member Author

Your changes LGTM. As always, thanks for the attention to detail @adeak!

@akaszynski akaszynski merged commit 53be529 into main Jun 14, 2022
@akaszynski akaszynski deleted the fix/bump_cache_version branch June 14, 2022 11:30
@MatthewFlamm
Copy link
Contributor

Turns out files are hard.

Had to handle another edge case where the retriever returns the exact same path as the requested filename. This edge case only occurs in testing, but I figure if someone wanted to implement a custom retriever for whatever reason in the future we should handle that as well rather than modifying the test.

I'm fine with the changes here since it is all private methods, and as long as we play by our rules everything should still work. But, I think the testing is flawed. If I'm understanding the issue is that a zip file retrieved is placed into the examples path by the retriever. Then the name clashes when we try to make a directory with the same name? We shouldn't allow for retrievers to return files in the example path as many bad things will happen. IMO we should raise an error instead and fix the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Uh-oh! Something isn't working as expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants