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
Add Content-Type headers to HTTP artifact upload calls #8048
Add Content-Type headers to HTTP artifact upload calls #8048
Conversation
mlflow/utils/mime_type_utils.py
Outdated
from mimetypes import guess_type | ||
|
||
# from mlflow.models.model import MLMODEL_FILE_NAME | ||
# from mlflow.projects._project_spec import MLPROJECT_FILE_NAME |
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.
Importing these constants from here creates a circular import. (artifacts -> utils -> models -> artifacts). Is there a good place to pull out these constants so that this util can reference them and it won't cause a circular import?
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.
Is there a good place to pull out these constants so that this util can reference
We currently don't have such a module. I think rewriting the constants in this file is ok for now.
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.
Does the following code work?
# TODO: Create a module to define constants to avoid circular imports and move MLMODEL_FILE_NAME and MLPROJECT_FILE_NAME in the module.
def get_mlodel_and_mlproject_filenames():
from mlflow.models.model import MLMODEL_FILE_NAME
from mlflow.projects._project_spec import MLPROJECT_FILE_NAME
return [MLMODEL_FILE_NAME, MLPROJECT_FILE_NAME]
_TEXT_EXTENSIONS = [
...,
*get_mlodel_and_mlproject_filenames()
]
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.
That still doesn't quite work because _TEXT_EXTENSIONS gets evaluated at import time and it triggers the circular dependency. But tweaking the idea a little, I can put the whole list in a function and that gets around it.
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.
Link to tweaked implementation: https://github.com/mlflow/mlflow/pull/8048/files#diff-cc0cc65505c5745295581af3a6ec580c1c734505dc96e748afa4170f09473042R7
mlflow/utils/mime_type_utils.py
Outdated
# from mlflow.projects._project_spec import MLPROJECT_FILE_NAME | ||
|
||
MLMODEL_FILE_NAME = "MLmodel" | ||
MLPROJECT_FILE_NAME = "mlproject" |
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.
(Just rewriting the constants here is a kludge while I wait for advice on the import issue)
@WillEngler Thank you for the contribution! Could you fix the following issue(s)? ⚠ DCO checkThe DCO check failed. Please sign off your commit(s) by following the instructions here. See https://github.com/mlflow/mlflow/blob/master/CONTRIBUTING.md#sign-your-work for more details. |
Documentation preview for 2616349 will be available here when this CircleCI job completes successfully. More info
|
f0da35b
to
33d4973
Compare
Was doing a pre-review and I think I still need to add a unit test to Update: this is ready for review. |
33d4973
to
1409230
Compare
1409230
to
a16beba
Compare
Fixed the failing tests. Ready for another CI run. |
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!
@WillEngler Can you resolve the conflict? |
75072c4
to
107d9bd
Compare
…actRepository._log_artifact. Signed-off-by: Will Engler <Engler.Will@gmail.com>
107d9bd
to
2616349
Compare
Thanks for the review @harupy! Resolved the conflicts |
@WillEngler Merged, thanks for the contribution! |
Related Issues/PRs
Resolves #8026
What changes are proposed in this pull request?
When the MLFlow client uploads artifacts via the HttpArtifactRepository, it will now send the proper HTTP Content-Type header. It pulls logic from #7827 that maps file extensions to Content-Types into a util module. Then it uses that util inside of
HTTPArtifactRepository
to add the right Content-Type header to artifact upload POST requests.How is this patch tested?
Does this PR change the documentation?
Release Notes
Is this a user-facing change?
Adds Content-Type headers for outbound artifact upload calls from the HTTPArtifactRepo.
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/recipes
: Recipes, Recipe APIs, Recipe configs, Recipe Templatesarea/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