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

Expose AWS Region to HDF5IO #1040

Merged
merged 26 commits into from
May 21, 2024
Merged

Expose AWS Region to HDF5IO #1040

merged 26 commits into from
May 21, 2024

Conversation

CodyCBakerPhD
Copy link
Contributor

@CodyCBakerPhD CodyCBakerPhD commented Jan 25, 2024

fix #1039

Motivation

Shows the two points of exposure required to fix this. Would be great if someone could take this PR over and properly expose it as an argument (not sure how tests should handle beyond just making sure it can pass it down...)

Also not sure why the h5py.File is instantiated twice

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Does the PR clearly describe the problem and the solution?
  • Have you reviewed our Contributing Guide?
  • Does the PR use "Fix #XXX" notation to tell GitHub to close the relevant issue numbered XXX when the PR is merged?

@CodyCBakerPhD CodyCBakerPhD changed the title hardcode region Expose AWS Region to HDF5IO Jan 25, 2024
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.86%. Comparing base (6a0f9d8) to head (fb6819e).
Report is 33 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1040      +/-   ##
==========================================
+ Coverage   88.72%   88.86%   +0.14%     
==========================================
  Files          45       45              
  Lines        9766     9774       +8     
  Branches     2773     2776       +3     
==========================================
+ Hits         8665     8686      +21     
+ Misses        779      774       -5     
+ Partials      322      314       -8     

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

@CodyCBakerPhD CodyCBakerPhD self-assigned this Feb 17, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review February 17, 2024 05:48
@CodyCBakerPhD
Copy link
Contributor Author

@oruebel Needed this PR (and probably a follow-up on PyNWB) for NWB Benchmarks

@rly @mavaylon1 I added a basic test but remaining lines of coverage I have no clue how to evoke, leaving it to you to finish that coverage if you care about it

@mavaylon1
Copy link
Contributor

I can take a look after I fly back this weekend. Ideally we want coverage. With @rly on vacation, I'll defer to @oruebel on that. But again, I'll take a look next week.

@mavaylon1
Copy link
Contributor

mavaylon1 commented May 17, 2024

@oruebel I think you are better suited for this PR. Can you take this one and then we talk to get me up to speed.

@oruebel
Copy link
Contributor

oruebel commented May 18, 2024

@rly can you help finish up this PR?

@rly
Copy link
Contributor

rly commented May 21, 2024

Review notes:

  • HDMF-zarr tests are failing for unrelated issues.

@rly rly enabled auto-merge (squash) May 21, 2024 01:58
@rly rly merged commit c18d1b3 into dev May 21, 2024
28 of 29 checks passed
@rly rly deleted the expose_aws_region branch May 21, 2024 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Expose aws_region to HDF5IO
4 participants