Integrating Mlflow. #116
Integrating Mlflow. #116
Conversation
… create the workspace if it does not exist
|
Check out this pull request on ReviewNB: https://app.reviewnb.com/microsoft/nlp/pull/116 You'll be able to see visual diffs and write comments on notebook cells. Powered by ReviewNB. |
| @@ -58,7 +59,8 @@ def test_load_pretrained_vectors_glove(): | |||
|
|
|||
| def test_load_pretrained_vectors_fasttext(): | |||
| dir_path = "temp_data/" | |||
| file_path = os.path.join(os.path.join(dir_path, "fastText"), "wiki.simple.bin") | |||
| file_path = os.path.join(os.path.join(dir_path, "fastText"), | |||
| "wiki.simple.bin") | |||
There was a problem hiding this comment.
is your editor auto changing these? you can update it to 120 or 100 since it seems to be at 80 and that is a bit too short
There was a problem hiding this comment.
the repo is configured at 79 (pep8) - black defaults to 88...
I'm for 120 :) but that seems to be far from any standard.
There was a problem hiding this comment.
Yeah. I have set my width to 80. I can move it to 100.
There was a problem hiding this comment.
In the gensen train file with multiple levels of indentation 80 just feels very less. The code almost looks vertical. 😄
There was a problem hiding this comment.
Yeah, mlflow uses 100. our sdk uses 119. 80 is due to a limitation in terminals from around the 1980s
| @@ -127,7 +127,7 @@ | |||
| }, | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I think the default workspace was pointed for this file store. Using previous changes made by @catherine667 here. She can answer it better.
AbhiramE
force-pushed
the
abhiram-mlflow
branch
2 times, most recently
from
054572f
to
80487c8
Compare
|
Rebased and forced pushed to the wrong branch! 😨 Fixed it with the latest push. |
| @@ -132,6 +132,13 @@ | |||
| "scrolled": true | |||
There was a problem hiding this comment.
this warning is unfortunate. do you know when you started getting it/ can you remove it from the notebook?
Reply via ReviewNB
There was a problem hiding this comment.
I got these warnings from the time I started running the gensen deep dive notebook on local. (A week ago) But before I did, I updated my current conda environment with the updated dependencies.
I can strip the output of this cell, if this warning needs to be hidden.
|
|
||
| # Keep track of indicies to train forward and backward jointly | ||
| if ( | ||
| "skipthought_next" in tasknames |
There was a problem hiding this comment.
you do not have to change tasknames to task_names but I really prefer it but since it is vendored I get why you would not
if all([task_name in task_names for task in [names, I, care, about]):
There was a problem hiding this comment.
I definitely agree with the suggested change. The change however is the tip of an iceberg. 😄 There's a lot that can be improved and refactored in the gensen code. For now though it would be great if we can pass on this.
There was a problem hiding this comment.
Changes look good. Notebook did not change much and the other updates are mostly simplifications.
If the 100 char per line PR lands beforehand I would update the PR to avoid having to make a follow up PR since you are currently familiar with all of your changes and it would be harder later.
Once the comments here are addressed I think it is good for check in.
Description
This PR integrates Mlflow for logging metrics. The change allows user's to locally log metrics like validation loss and display it in a UI using
mlflow-ui.The change also supports AzureML-mlflow. Changes made to gensen deep dive notebook utilize this package to track mlflow logged metrics in the Azure portal.
This change helps us move away from using AzureML in the utils. By using Mlflow for logging we provide the flexibility to opt in/opt out of AzureML for the users.
Related Issues
Checklist: