-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Support filtering runs by start_time
#4922
Conversation
start_time
start_time
cauese an internal server error
start_time
cauese an internal server errorstart_time
mlflow/utils/search_utils.py
Outdated
VALID_STRING_ATTRIBUTE_COMPARATORS = set(["!=", "=", LIKE_OPERATOR, ILIKE_OPERATOR]).union( | ||
VALID_METRIC_COMPARATORS | ||
) |
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.
I suppose it's a bit odd to do > / < lexical comparison on e.g. artifact location, but it doesn't seem outwardly harmful.
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.
I agree. Ideally, only the start_time column should be allowed to use [">", ">=", "!=", "=", "<", "<="]
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.
VALID_STRING_ATTRIBUTE_COMPARATORS
is used here:
mlflow/mlflow/utils/search_utils.py
Lines 340 to 349 in 4d4b462
@classmethod | |
def is_attribute(cls, key_type, comparator): | |
if key_type == cls._ATTRIBUTE_IDENTIFIER: | |
if comparator not in cls.VALID_STRING_ATTRIBUTE_COMPARATORS: | |
raise MlflowException( | |
"Invalid comparator '{}' not one of " | |
"'{}".format(comparator, cls.VALID_STRING_ATTRIBUTE_COMPARATORS) | |
) | |
return True | |
return False |
We could:
- Define
VALID_STARTTIME_ATTRIBUTE_COMPARATORS
- Then fix
is_attribute
:
@classmethod
def is_attribute(cls, key_type, key_name, comparator):
if key_type == cls._ATTRIBUTE_IDENTIFIER:
if comparator not in cls.VALID_STARTTIME_ATTRIBUTE_COMPARATORS:
raise ...
elif comparator not in cls.VALID_STRING_ATTRIBUTE_COMPARATORS:
raise MlflowException(
"Invalid comparator '{}' not one of "
"'{}".format(comparator, cls.VALID_STRING_ATTRIBUTE_COMPARATORS)
)
return True
return False
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.
How about VALID_NUMERIC_ATTRIBUTE_COMPARATORS
?
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.
Sounds good to me!
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!
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
I encountered another issue. Combining comparison expressions causes an error: In this image, I set the start-time filter to I'm investingating why this is happening. The request payload in the image above looks like: {
"experiment_ids": [
"0"
],
"filter": "metrics.m2 > 0.2 && attributes.start_time >= 1634865257337",
"run_view_type": "ACTIVE_ONLY",
"max_results": 100,
"order_by": [
"attributes.start_time DESC"
],
"page_token": null
} The filter expressions are joined by Found the culprit: mlflow/mlflow/server/js/src/experiment-tracking/components/ExperimentPage.js Lines 204 to 210 in 035dcbd
|
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
mlflow/utils/search_utils.py
Outdated
@@ -186,7 +188,9 @@ def _get_value(cls, identifier_type, token): | |||
error_code=INVALID_PARAMETER_VALUE, | |||
) | |||
elif identifier_type == cls._ATTRIBUTE_IDENTIFIER: | |||
if token.ttype in cls.STRING_VALUE_TYPES or isinstance(token, Identifier): | |||
if key == "start_time": |
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.
if key == "start_time": | |
if key in cls.NUMERIC_ATTRIBUTES: |
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy 17039389+harupy@users.noreply.github.com
What changes are proposed in this pull request?
Support filtering runs by
start_time
.How is this patch tested?
Unit tests
Release Notes
Is this a user-facing change?
(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)
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