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

Use a github action to centralize gin data preparation #1095

Merged
merged 7 commits into from
Sep 23, 2024

Conversation

h-mayorquin
Copy link
Collaborator

While working on #1013 I was having trouble getting the data right from the caches (see the diff in that PR). Centralizing the logic should make this kind of problem easier to deal with.

This is a partial move in the direction of #993.

@h-mayorquin h-mayorquin added the CI label Sep 18, 2024
@h-mayorquin h-mayorquin self-assigned this Sep 18, 2024
@h-mayorquin h-mayorquin changed the title Centralized gin data preparation in an action Use a github action to centralize gin data preparation Sep 18, 2024
@h-mayorquin h-mayorquin marked this pull request as ready for review September 18, 2024 07:58
Copy link
Member

@pauladkisson pauladkisson left a comment

Choose a reason for hiding this comment

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

Looks good, this PR should probably delete update-s3-testing-data.yml to avoid confusion.

@h-mayorquin
Copy link
Collaborator Author

Looks good, this PR should probably delete update-s3-testing-data.yml to avoid confusion.

What possible confusion do you in mind?

@pauladkisson
Copy link
Member

Looks good, this PR should probably delete update-s3-testing-data.yml to avoid confusion.

What possible confusion do you in mind?

update-s3-testing-data.yml is disabled, and actions/load-data seems to be a replacement (is it not?), so we should delete update-s3-testing-data.yml to avoid maintaining vestigial workflows that aren't in use.

@h-mayorquin
Copy link
Collaborator Author

Looks good, this PR should probably delete update-s3-testing-data.yml to avoid confusion.

What possible confusion do you in mind?

update-s3-testing-data.yml is disabled, and actions/load-data seems to be a replacement (is it not?), so we should delete update-s3-testing-data.yml to avoid maintaining vestigial workflows that aren't in use.

No, it is not meant to replace it. This assumes the data is already on the s3 and loads it either from the cache or from the s3 if the former fails. At the moment we don't have a mechanism to automatically transfer the data from gin to s3. We should meet at brainstorm that.

@pauladkisson
Copy link
Member

No, it is not meant to replace it. This assumes the data is already on the s3 and loads it either from the cache or from the s3 if the former fails.

I see. update-testing-data.yml on roiextractors is more like load-data/action.yml than update-s3-testing-data.yml, so that's why I misunderstood.

At the moment we don't have a mechanism to automatically transfer the data from gin to s3. We should meet at brainstorm that.

gin is very finnicky, so we might just be better off keeping our current (manual) system.

@h-mayorquin
Copy link
Collaborator Author

gin is very finnicky, so we might just be better off keeping our current (manual) system.

What troubles have you experienced? Not that I disagree.

@h-mayorquin h-mayorquin enabled auto-merge (squash) September 23, 2024 18:21
@pauladkisson
Copy link
Member

What troubles have you experienced?

Failed downloads mostly.

@h-mayorquin h-mayorquin merged commit f73bbee into main Sep 23, 2024
39 checks passed
@h-mayorquin h-mayorquin deleted the centralize_data_access branch September 23, 2024 19:35
Copy link

codecov bot commented Sep 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.58%. Comparing base (36464df) to head (0a82274).
Report is 15 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1095      +/-   ##
==========================================
+ Coverage   90.44%   90.58%   +0.13%     
==========================================
  Files         129      129              
  Lines        8055     8164     +109     
==========================================
+ Hits         7285     7395     +110     
+ Misses        770      769       -1     
Flag Coverage Δ
unittests 90.58% <ø> (+0.13%) ⬆️

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

see 2 files with indirect coverage changes

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.

2 participants