[WIP] Some cleanups #790
[WIP] Some cleanups #790
Conversation
| @@ -208,7 +208,7 @@ def set_terminated(self, run_id, status=None, end_time=None): | |||
| Defaults to "FINISHED". | |||
| :param end_time: If not provided, defaults to the current time.""" | |||
| end_time = end_time if end_time else int(time.time() * 1000) | |||
| status = status if status else "FINISHED" | |||
| status = status if status else RunStatus.to_string(RunStatus.FINISHED) | |||
There was a problem hiding this comment.
flipping existing usage of free flowing strings to pre-defined constants. ^^this^^ is one example. More further down..
| @@ -150,7 +150,7 @@ def start_run(run_uuid=None, experiment_id=None, source_name=None, source_versio | |||
| return _active_run_stack[-1] | |||
|
|
|||
|
|
|||
| def end_run(status="FINISHED"): | |||
| def end_run(status=RunStatus.to_string(RunStatus.FINISHED)): | |||
There was a problem hiding this comment.
n00b questions for you: is the goal to have functions take in the enum and not the string? seems true for create_run above but not for this one (end_run). or these functions are meant for different contexts so we want the enum for one and the string for the other ?
There was a problem hiding this comment.
No! that is a good observation. This PR matches the behavior of SqlAlchemyStore's API with that of FileStore since the caller for them are the same client/fluent depending on the context and they will instantiate the backend store. That said, it does make sense to take audit of these APIs and maintain constancy and see if some of these APIs need to be changed (it could result in a breaking API change)
My preference is enums -- and use strings for readability -- logs, messages, UI ... etc.
There was a problem hiding this comment.
but we do want this end_run function to take in a string because it's exposed to the end user, right? (so this PR is good to go?) and eventually we want all the internal methods to take in the enum.
There was a problem hiding this comment.
Yeah. I agree that user facing functions to take in string-y fields and we can check for valid values.
This change only replaces the default value of status from constant "FINISHED" to RunStatus.to_string(RunStatus.FINISHED) <-- still a string argument.
|
LGTM
|
- runs.source_version column increased size to 50 - runs.name: column removed uniqueness constraint, defaults to "" (in entities.run_info) if not provided - missing default system tags to match FileStore
Getting some conformity with usage for RunStatus and source. Some paths were using strings while others were using the enum values.