-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Parallelize download of artifacts and improve performance of downloading large models. #5359
Conversation
Add one line in the model signature introduction section and add link to detailed section in the introduction. Signed-off-by: Yogita Mehta <yogita.mehta@databricks.com>
Signed-off-by: Yogita Mehta <yogita.mehta@databricks.com>
…g sphinx build locally. Signed-off-by: Yogita Mehta <yogita.mehta@databricks.com>
…e while logging model to avoid float precision errors. Signed-off-by: Yogita Mehta <yogita.mehta@databricks.com>
Signed-off-by: Yogita Mehta <yogita.mehta@databricks.com>
…formance for downloading large models. Signed-off-by: Yogita Mehta <yogita.mehta@databricks.com>
Signed-off-by: Yogita Mehta <yogita.mehta@databricks.com>
Signed-off-by: Yogita Mehta <yogita.mehta@databricks.com>
Signed-off-by: Yogita Mehta <yogita.mehta@databricks.com>
Signed-off-by: Yogita Mehta <yogita.mehta@databricks.com>
Signed-off-by: Yogita Mehta <yogita.mehta@databricks.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mehtayogita This is awesome! Thanks for bringing download parallelism to all of the artifact repositories across MLflow. LGTM; feel free to merge once you've supplemented unit tests & conducted manual testing and updated the PR description with a manual test plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really excited about the contribution! 2 small questions
Also, it'd be good to add some unit testing here to verify edge cases - how do we plan on doing that? |
Thanks for the quick review! Will confirm this is working as intended with manual testing, add tests and request another round of review. Thanks!
Thanks for the quick review! Yes the CL needs to be polished more and have unit tests. The parallelism is currently tested using tests/store/artifact/test_databricks_artifact_repo.py, while it is better to be part of tests/store/artifact/test_artifact_repo.py. The edge cases will be tested in there. I will first confirm this change is working as intended with manual testing and will then update the PR with tests and address TODOs left in the code. Thanks! |
…_artifacts function. Signed-off-by: Yogita Mehta <yogita.mehta@databricks.com>
…ential per one artifact download. Signed-off-by: Yogita Mehta <yogita.mehta@databricks.com>
Updated the PR to include the unit tests and confirmed performance improvements with manual testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for the change - great first contribution to MLflow!
Btw, could you add a bit more detail about the performance test you did and include a reference to the performance improvement in the release note? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @mehtayogita !
Added details. |
What changes are proposed in this pull request?
Parallelize download function for ArtifactRepository.
How is this patch tested?
Unit testing
Manually testing with models having large artifacts. For a model with ~200 artifacts, this change provided 2x improvement. The code version at HEAD took 109 secs to load the model using ModelsArtifactRepository, while the updated code in this PR took 66.56 secs.
Does this PR change the documentation?
ci/circleci: build_doc
check. If it's successful, proceed to thenext step, otherwise fix it.
Details
on the right to open the job page of CircleCI.Artifacts
tab.docs/build/html/index.html
.Release Notes
Is this a user-facing change?
Support parallel download of artifacts. Fix issue: #5338
What component(s), interfaces, languages, and integrations does this PR affect?
Components
area/artifacts
: Artifact stores and artifact loggingarea/build
: Build and test infrastructure for MLflowarea/docs
: MLflow documentation pagesarea/examples
: Example codearea/model-registry
: Model Registry service, APIs, and the fluent client calls for Model Registryarea/models
: MLmodel format, model serialization/deserialization, flavorsarea/projects
: MLproject format, project running backendsarea/scoring
: MLflow Model server, model deployment tools, Spark UDFsarea/server-infra
: MLflow Tracking server backendarea/tracking
: Tracking Service, tracking client APIs, autologgingInterface
area/uiux
: Front-end, user experience, plotting, JavaScript, JavaScript dev serverarea/docker
: Docker use across MLflow's components, such as MLflow Projects and MLflow Modelsarea/sqlalchemy
: Use of SQLAlchemy in the Tracking Service or Model Registryarea/windows
: Windows supportLanguage
language/r
: R APIs and clientslanguage/java
: Java APIs and clientslanguage/new
: Proposals for new client languagesIntegrations
integrations/azure
: Azure and Azure ML integrationsintegrations/sagemaker
: SageMaker integrationsintegrations/databricks
: Databricks integrationsHow should the PR be classified in the release notes? Choose one:
rn/breaking-change
- The PR will be mentioned in the "Breaking Changes" sectionrn/none
- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/feature
- A new user-facing feature worth mentioning in the release notesrn/bug-fix
- A user-facing bug fix worth mentioning in the release notesrn/documentation
- A user-facing documentation change worth mentioning in the release notes