Skip to content

Conversation

mmarinwilcke
Copy link
Contributor

Add initial metadata config on the model run when creating it

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@tomislav-peharda
Copy link
Contributor

Hey, I don't have plenty of context on here, so would rather have someone else give a stamp / review

@kkim-labelbox
Copy link
Contributor

Looks like the build is failing because your code is stale. It'll pass after you pull the latest changes

@mmarinwilcke mmarinwilcke requested a review from jtsodapop August 10, 2022 13:46
Copy link
Contributor

@jtsodapop jtsodapop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

res = self.client.execute(query_str, {
name_param: name,
config_param: config,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, does this work okay when the param is None?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, let me test that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're at it, could you add a test case for config being empty as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, thats what I meant! haha

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good

@mmarinwilcke mmarinwilcke merged commit 5b4b4bf into develop Aug 10, 2022
@jtsodapop jtsodapop deleted the mmw/create-model-run-with-config branch September 14, 2022 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants