Skip to content
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

Add proportion_samples to generated api client #914

Merged

Conversation

guarin
Copy link
Contributor

@guarin guarin commented Aug 25, 2022

Adds api generated code for proportion_samples option.

TODO

@guarin guarin force-pushed the guarin-lig-1661-add-proportion_samples-to-openapi-specs branch from 4f926f8 to 3858722 Compare August 25, 2022 14:25
Comment on lines -166 to -175
def test__selection_config_from_dict__missing_n_samples() -> None:
cfg = {
"strategies": [
{"input": {"type": "EMBEDDINGS"}, "strategy": {"type": "DIVERSIFY"}},
]
}
with pytest.raises(ValueError, match="Invalid value for `n_samples`, must not be `None`"):
api_workflow_compute_worker._selection_config_from_dict(cfg)


Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test no longer makes sense as n_samples is now optional

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have two tests instead that checks that both n_samples and proportion_samples can be set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, great catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually verified by the test__selection_config_from_dict test.

@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #914 (3858722) into master (32338b9) will increase coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #914      +/-   ##
==========================================
+ Coverage   89.22%   89.36%   +0.13%     
==========================================
  Files          97       97              
  Lines        4335     4335              
==========================================
+ Hits         3868     3874       +6     
+ Misses        467      461       -6     
Impacted Files Coverage Δ
lightly/api/api_workflow_download_dataset.py 92.94% <0.00%> (+7.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@MalteEbner MalteEbner left a comment

Choose a reason for hiding this comment

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

LGTM

Perhaps you have already though about it, but could we also have the move the_verify_stopping_condition_set() method here and call it before sending the SelectionConfig, e.g. after calling _selection_config_from_dict()?

@guarin
Copy link
Contributor Author

guarin commented Aug 25, 2022

Perhaps you have already though about it, but could we also have the move the_verify_stopping_condition_set() method here and call it before sending the SelectionConfig, e.g. after calling _selection_config_from_dict()?

I didn't think about it :D But if we do this then the config is only verified if it is set by the pip client. It would not verified if it is set by the API or the webapp.

@guarin guarin merged commit 097927c into master Aug 25, 2022
@guarin guarin deleted the guarin-lig-1661-add-proportion_samples-to-openapi-specs branch August 25, 2022 15:22
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.

None yet

2 participants