Provide default implementations of AbstractStore log methods #1051
Conversation
|
This is a great catch. However, does it make sense to mark all API level methods in abstract store as There are a few more methods in
|
|
Hey, thanks for the comments.
The other three should be abstract - I'll update those on this PR. |
|
Correction - |
acroz
force-pushed
the
abstract-store-default-log-implementations
branch
2 times, most recently
from
97dc25c
to
97b2685
Compare
|
@mparkhe I think I've resolved the outstanding issues on this PR, but the tests are failing for a seemingly unrelated issue with Sagemaker tests. Is this an issue on master? |
Yes. @smurching and @dbczumar are looking into it. |
| @@ -224,17 +224,6 @@ def search_runs(self, experiment_ids, search_filter, run_view_type): | |||
| response_proto = self._call_endpoint(SearchRuns, req_body) | |||
| return [Run.from_proto(proto_run) for proto_run in response_proto.runs] | |||
|
|
|||
| def list_run_infos(self, experiment_id, run_view_type): | |||
tests/store/test_abstract_store.py
Outdated
| from mlflow.entities import ViewType | ||
|
|
||
|
|
||
| class ConcreteStore(AbstractStore): |
There was a problem hiding this comment.
🤣 that name.
Can we rename this to something like TestAbstractStoreImpl
There was a problem hiding this comment.
Done - though I do miss the ConcreteStore 😁
I made it AbstractStoreTestImpl as classes named Test* are inspected by pytest for test methods, I believe.
| def log_batch(self, run_id, metrics, params, tags): | ||
| raise NotImplementedError() | ||
|
|
||
|
|
There was a problem hiding this comment.
Would be good to write tests for non-abstract methods in AbstractStore. You have covered log_* methods--can we write one for list_run_infos as well?
There was a problem hiding this comment.
Done - also added tests for get_experiment_by_name to complete the set.
acroz
force-pushed
the
abstract-store-default-log-implementations
branch
from
97b2685
to
14e9555
Compare
|
@mparkhe Have addressed your comments and now rerunning tests. |
The abstract store has methods for logging individual metrics, params and tags, however since these are not marked as abstract methods they're not currently required to be implemented in tracking store implementations. This change provides default implementations of `log_metric`, `log_param` and `set_tag` that proxy calls to the more general `log_batch` added by @smurching in mlflow#950. Following this change, as long as stores implement `log_batch`, the above methods need not be provided.
To make construction of the abstract class work
The implementation is the same as that inherited from `AbstractStore`, so override is redundant
acroz
force-pushed
the
abstract-store-default-log-implementations
branch
from
14e9555
to
36712c7
Compare
|
@acroz Merging now! |
…1051) * Provide default implementations of AbstractStore log methods The abstract store has methods for logging individual metrics, params and tags, however since these are not marked as abstract methods they're not currently required to be implemented in tracking store implementations. This change provides default implementations of `log_metric`, `log_param` and `set_tag` that proxy calls to the more general `log_batch` added by @smurching in mlflow#950. Following this change, as long as stores implement `log_batch`, the above methods need not be provided. * Mark other required methods as abstract * Patch all abstract methods To make construction of the abstract class work * Forgot to add fixture to test functions * list_run_infos has a default implementation and so doesn't need to be abstract * Remove RestStore.list_run_infos The implementation is the same as that inherited from `AbstractStore`, so override is redundant * Don't need to patch list_run_infos any more * Ignore unused argument warning with pytest fixture * Need to subclass to pass Python 2.7 tests * Fix linting errors * Remove duplicate method. * Rename test store implementation * Add test for get_experiment_by_name * Add test for list_run_infos * Add missing assertion
The abstract store has methods for logging individual metrics, params and tags, however since these are not marked as abstract methods they're not currently required to be implemented in tracking store implementations.
This change provides default implementations of
log_metric,log_paramandset_tagthat proxy calls to the more generallog_batchadded by @smurching in #950.Following this change, as long as stores implement
log_batch, the above methods need not be provided.