Native MLFlow tracking server support #35
Conversation
Codecov Report
@@ Coverage Diff @@
## master #35 +/- ##
==========================================
+ Coverage 51.51% 51.84% +0.32%
==========================================
Files 30 31 +1
Lines 2972 3148 +176
==========================================
+ Hits 1531 1632 +101
- Misses 1441 1516 +75
Continue to review full report at Codecov.
|
|
todo list:
|
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
|
@ajslone , here's an idea for how to handle the wrapper script in a slightly cleaner way. from pkg_resources import resource_filename
def wrapper_script_path():
return resource_filename('caliban.resources', FILEPATH)this is from a different project I'm working on; but if you modify the script to take arguments instead of actually templating it, then you can put the script into the advantage is that we won't print the entire script out to stdout as the image is building. I think this solves another issue for us too... the issue of injecting run-time environment variables into the container. Since we can send in kv pairs of env variable / value, have this wrapper script take them and set them, and then pass the rest of its flags into the user's script, this should solve the problem of injecting run and experiment IDs into the container. (I think if we do this it's worth promoting an |
|
@sritchie agree this is a far better way to do this, I'll push a change to implement this way. |
setup.py
Outdated
| @@ -56,7 +56,8 @@ def readme(): | |||
| # dep that commentjson brings in. Delete once this is merged: | |||
| # https://github.com/vaidik/commentjson/pull/33/files | |||
| 'lark-parser>=0.7.1,<0.8.0', | |||
| 'SQLAlchemy>=1.3.11', | |||
| 'SQLAlchemy==1.3.11', | |||
| 'mlflow>=1.9.1', | |||
There was a problem hiding this comment.
I don't see where we use this dependency, can we remove?
There was a problem hiding this comment.
Right, this was a relic from earlier tests where we were calling the mlflow api in caliban. Will remove.
… wrapper using json args
|
these latest changes are a WIP, will break this up once I can verify everything is working |
…xt, minor change to train.py
This PR: Adds the ability to inject a launcher script into the container, with the ability to intercept arguments, set environment variables and start up a service that will be available for the duration of the container's task. This is the conclusion of a port of #35 . Co-authored-by: Ambrose Slone <[email protected]>
PR for MLFlow integration.
todo list: