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

Download model files in parallel when < X files #122

Merged
merged 10 commits into from
May 31, 2024
Merged

Conversation

rosbo
Copy link
Contributor

@rosbo rosbo commented May 17, 2024

@rosbo
Copy link
Contributor Author

rosbo commented May 17, 2024

@jeward414 I will add tests on Monday but here is what we did together. I also added the parallel download part.

@rosbo
Copy link
Contributor Author

rosbo commented May 24, 2024

Note: integration test is failing until https://github.com/Kaggle/kaggleazure/pull/29608 is merged (@stevemessick is working on the backend fix). I will wait for the change to be deployed before merging this PR.

@rosbo rosbo marked this pull request as ready for review May 24, 2024 16:40
@rosbo rosbo requested review from mohami2000 and jeward414 May 24, 2024 16:40
@rosbo rosbo force-pushed the parallel-direct-download branch from fe1c465 to 695b4c1 Compare May 24, 2024 23:32
@rosbo rosbo force-pushed the parallel-direct-download branch from 5b84f34 to f91ae77 Compare May 31, 2024 16:23
@rosbo
Copy link
Contributor Author

rosbo commented May 31, 2024

I found why the integration tests were failing on CI but not when running locally.

There is a kaggle-api-enable-pagination feature flag for the new ListModelInstanceVersionFilesHandler. It is currently released to 50%. The integrationtester on CI didn't fall into this bucket so it was always getting an empty response when listing files: https://github.com/Kaggle/kaggleazure/blob/dc89139c44f4d0ab33c1331c4326340619b3f6a0/Kaggle.Services.Models/Handlers/ModelApiService/V1/ListModelInstanceVersionFilesHandler.cs#L86

@stevemessick Are you planning to turn the feature flag to public soon? Also, did we need the feature flag for the model handler given it is new?

@stevemessick
Copy link

@rosbo That FF was supposed to have been 100% but the update didn't stick. I just verified it is now 100%.

The FF will be removed by https://github.com/Kaggle/kaggleazure/pull/29772.

@stevemessick
Copy link

I re-ran the failing check, and it is passing now.

@rosbo rosbo merged commit 63df5fe into main May 31, 2024
7 checks passed
@rosbo rosbo deleted the parallel-direct-download branch May 31, 2024 17:59
@rosbo
Copy link
Contributor Author

rosbo commented May 31, 2024

Thank you @stevemessick 👏

rosbo added a commit that referenced this pull request Jun 5, 2024
We started using the `ListModelInstanceVersionFiles` in #122 (not yet
released but merged). However, I noticed this doesn't work for
unauthenticated users.

I added an integration test to ensure we don't regress on this once the
backend is fixed.

http://b/341160276
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.

4 participants