Separate DLL checking in windows

[peterhollender]

Changes the way windows checks for DLLs to prevent re-downloading on every import and double-downloading of DLLs.

Closes #419

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.59%. Comparing base (41f00ee) to head (16fcf4a).
Report is 25 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #420   +/-   ##
=======================================
  Coverage   71.58%   71.59%           
=======================================
  Files          46       46           
  Lines        6736     6738    +2     
  Branches     1495     1496    +1     
=======================================
+ Hits         4822     4824    +2     
  Misses       1345     1345           
  Partials      569      569           

Flag	Coverage Δ
3.10	71.79% <100.00%> (+<0.01%)	⬆️
3.11	71.79% <100.00%> (+<0.01%)	⬆️
3.12	71.79% <100.00%> (+<0.01%)	⬆️
3.9	71.57% <100.00%> (+<0.01%)	⬆️
macos-latest	71.53% <100.00%> (+<0.01%)	⬆️
ubuntu-latest	71.56% <100.00%> (+<0.01%)	⬆️
windows-latest	71.57% <100.00%> (+<0.01%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[waltsims]

Thanks for the PR @peterhollender,

I'll have a look.

[peterhollender]

How would you like a windows-specific platform test to be implemented? I see the Codecov report flagged the if PLATFORM == "windows" condition.

[waltsims]

Hey @peterhollender,

I added a failing test to cover the double download case.

I think we can simplify the PR before merging it in. I believe the following patch is all we need to fix this issue.

def get_windows_release_urls(architecture: str) -> list:
    specific_filenames = [EXECUTABLE_PREFIX + architecture + ".exe"]
    if architecture == "omp":
        specific_filenames += WINDOWS_DLLS
    release_urls = [PREFIX.format(architecture.upper(), PLATFORM.lower()) + filename for filename in specific_filenames]
    return release_urls

Could you confirm that using only this patch (along with the updated import statement) works for you?

Walter

[waltsims]

I think codecov's is having an issue tonight. We likely hit the rate-limit (an open issue). I hope you don't mind that I pushed to your fork.

[waltsims]

please see my comments.

[peterhollender]

Hey @peterhollender,

I added a failing test to cover the double download case.

I think we can simplify the PR before merging it in. I believe the following patch is all we need to fix this issue.

def get_windows_release_urls(architecture: str) -> list:
    specific_filenames = [EXECUTABLE_PREFIX + architecture + ".exe"]
    if architecture == "omp":
        specific_filenames += WINDOWS_DLLS
    release_urls = [PREFIX.format(architecture.upper(), PLATFORM.lower()) + filename for filename in specific_filenames]
    return release_urls

Could you confirm that using only this patch (along with the updated import statement) works for you?

Walter

Yeah, that simpler patch works. I started with something like that and then made it more complicated (I think I was trying to better organize it in case we ever wanted to install just the OMP or just the CUDA binaries, but I didn't carry that notion all the way through. If we were going separate them out, you'd probably want to create a whole new kspaceFirstOrder-DLL-windows repo to hold them.

One thing to note about this patch is that by setting the URL of the DLLs to the OMP version, everyone who has it installed is going to trigger one more re-install the next time they import, since they will all have double-installed the CUDA version over the OMP version and left the CUDA URL in their metadata.

[waltsims]

Yeah in the future it would be great to clean up the binary releases and repositories. Good point on that last update. If you'd prefer setting the DLL repo to CUDA that's fine by me.