BAI-1255 MLFlow Model Imports #1341
Conversation
lib/python/src/bailo/helper/model.py
Outdated
| :param visibility: Visibility of model on Bailo, using ModelVisibility enum (e.g Public or Private), defaults to None | ||
| :return: A model object | ||
| """ | ||
| if mlflow: |
There was a problem hiding this comment.
Should be ml_flow given the boolean variable.
lib/python/src/bailo/helper/model.py
Outdated
| else: | ||
| sel_model = res[0] | ||
|
|
||
| if not sel_model: |
There was a problem hiding this comment.
Should use if sel_model is None
lib/python/src/bailo/helper/model.py
Outdated
| if version: | ||
| for model in res: | ||
| if model.version == version: | ||
| sel_model = model | ||
| break | ||
| sel_model = None | ||
| else: | ||
| sel_model = res[0] |
There was a problem hiding this comment.
sel_model is possibly unset which will cause a NameError when trying to access it. Instead you should move sel_model = None above if version and remove the break.
lib/python/src/bailo/helper/model.py
Outdated
| ) | ||
|
|
||
| mlflow_run = mlflow_client.get_run(run_id) | ||
| artifact_uri = mlflow_run.info.artifact_uri |
There was a problem hiding this comment.
Possibly undefined. Could there be some error handling to catch this?
lib/python/src/bailo/helper/model.py
Outdated
| ) | ||
|
|
||
| name = sel_model.name | ||
| description = sel_model.description + " Imported from MLFlow." |
There was a problem hiding this comment.
MLFlow sets description as '' by default so this shouldn't be an issue
lib/python/tests/conftest.py
Outdated
| def mlflow_uri(): | ||
| return "http://127.0.0.1:5001" |
There was a problem hiding this comment.
Using a fixture is only benificial if you're generating something. Here you can just use it as a constant as there's no additional computation.
There was a problem hiding this comment.
Done. Used pytest_configure to set a global variable
lib/python/src/bailo/helper/model.py
Outdated
| :param visibility: Visibility of model on Bailo, using ModelVisibility enum (e.g Public or Private), defaults to None | ||
| :return: A model object | ||
| """ | ||
| if ml_flow: |
There was a problem hiding this comment.
Invert these checks so that the error message is location by the if statement. E.g. instead of:
if x:
if y:
if z:
// do a
else:
throw 'z'
else:
throw 'y'
else:
throw 'x'
Instead:
if not x:
throw 'x'
if not y:
throw 'y'
if not z:
throw 'z'
// do a
At the moment I have to scroll through ~30 lines of code to see where the source is.
No description provided.