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
Improve string representations of entities #371
Conversation
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.
Very cool implementation. Minor comment about property ordering in output.
mlflow/entities/_mlflow_object.py
Outdated
return self.printer.pformat(obj) | ||
|
||
def serialize_entity(self, entity): | ||
return ", ".join(["%s=%s" % (self.serialize(key), self.serialize(value)) |
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.
Can we iterate this in the order specified _properties()
method for each entity.
Here's why with an example. Experiment would read more intuitively this way --
[<mlflow.entities.experiment.Experiment: 'experiment_id'=0, 'name'='Default', 'artifact_location'='/Users/sid/code/mlflow/mlruns/0'>]
Rather than hash sorted order.
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.
+1, that would be nice. Also don't put quotes around the key -- basically change the self.serialize(key)
to just key
.
mlflow/entities/_mlflow_object.py
Outdated
super(_MLflowObjectPrinter, self).__init__() | ||
self.printer = pprint.PrettyPrinter() | ||
|
||
def serialize(self, obj): |
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.
👍 very neat and tight implementation!
mlflow/entities/_mlflow_object.py
Outdated
return serialize(self) | ||
|
||
|
||
def serialize(obj): |
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 as a note, I think "serialize" can mean converting to formats other than string representation, so I would call this to_string or pretty_print or something.
mlflow/entities/_mlflow_object.py
Outdated
|
||
def serialize(self, obj): | ||
if isinstance(obj, _MLflowObject): | ||
return "<%s: %s>" % (get_classname(obj), self.serialize_entity(obj)) |
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'd include just the last part of the class name: Experiment instead of mlflow.entities.experiment.Experiment.
* rename serialize -> to_string * order fields based on self._properties * don't use quotes in keys * show final segment of classname
Currently, MLflow entities use default Python string serialization, resulting in hard-to-interpret output like:
This PR improves entities' string representations to include a listing of their fields (and recursive serialization of lists of entities, e.g. lists of metrics/params within a run). The above now results in:
Runs now appear as:
Feedback is welcome - run serialization is still somewhat unwieldy due to the number of fields in a run.