-
Notifications
You must be signed in to change notification settings - Fork 23
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
Data Engine: Autolog to MLflow on datasource queries #463
Conversation
Changes to be done:
|
ba02535
to
aa07a91
Compare
counter_value = _autolog_counters[source_name] | ||
_autolog_counters[source_name] += 1 | ||
artifact_name = f"autolog_{source_name}_{counter_value}.dagshub.json" | ||
threading.Thread(target=self.log_to_mlflow, kwargs={"artifact_name": artifact_name}).start() |
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.
Would be wise to pass along the active run as a parameter
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.
And, are you very sure that log_to_mlflow
is safe to do async?
The datasource object won't be modified, or otherwise be made into garbage by the time log_to_mlflow
gets executed?
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.
The datasource object is deep copied on every modification, so log_to_mlflow
is guaranteed to be working with the same object
|
||
artifact_name = f"autolog_{source_name}_{now_time}_{uuid_chunk}.dagshub.json" | ||
threading.Thread( | ||
target=self.log_to_mlflow, kwargs={"artifact_name": artifact_name, "run": mlflow.active_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.
Might want to reuse the active_run()
from line 753
Implemented in this PR:
Left to implement:
Caveats/bugs: