Make use_training_labels positional required #11529
Conversation
sdk/formrecognizer/azure-ai-formrecognizer/azure/ai/formrecognizer/_form_training_client.py
Outdated
Show resolved
Hide resolved
…izer/_form_training_client.py
| @@ -79,8 +79,8 @@ def __init__(self, endpoint, credential, **kwargs): | |||
| ) | |||
|
|
|||
| @distributed_trace | |||
| def begin_train_model(self, training_files_url, use_training_labels=False, **kwargs): | |||
| # type: (str, Optional[bool], Any) -> LROPoller | |||
| def begin_train_model(self, training_files_url, use_training_labels, **kwargs): | |||
There was a problem hiding this comment.
Can you update the changelog?
| self.assertEqual(len(model.training_documents), 1) | ||
| self.assertEqual(model.training_documents[0].document_name, "subfolder/Form_6.jpg") # we filtered for only subfolders | ||
|
|
||
| with self.assertRaises(HttpResponseError): | ||
| model = await client.train_model(training_files_url=container_sas_url, prefix="xxx") | ||
| model = await client.train_model(training_files_url=container_sas_url, use_training_labels=False, prefix="xxx") |
There was a problem hiding this comment.
Can you update sample_train_model_without_labels (the sync and async)? they don't have use_training_labels set, and they also include a comment that the default is use_training_labels=False`
Can you also run the samples that call train_model (or begin_train_model), just to make 100% sure they pass with this new change? thanks!
There was a problem hiding this comment.
Are we missing tests that use training labels (user_training_labels=True)?
There was a problem hiding this comment.
we have the tests - just didnt need to update them
There was a problem hiding this comment.
Grudgingly approve :).
Yes, there is an order between URL and use_training_label. But if we ever have another required parameter, then it is unlikely that there is a logical order. But then again, that would also be a breaking change unless the new parameter was optional.
I think we can safely assume there won't be breaking changes after GA - atleast i know we will try to avoid - so another required param after GA seems highly unlikely. |
|
/azp run pythob - formrecognizer - ci |
|
No pipelines are associated with this pull request. |
|
/check-enforcer evaluate |
…into feature/text_analytics_v3.0 * 'master' of https://github.com/Azure/azure-sdk-for-python: Release azure mgmt containerregistry (#11460) Prepare core 1.6.0 on master (#11610) Add force parameter to SwaggerToSdk CLI (#11609) LRO continuation_token (#10801) Remove unecessary import (#11606) Fix Cleanup failing on 3.8.3 (#11607) remove DataSourceCredentials (#11605) Search synonym map (#11590) Fix copy tests (#11594) Make use_training_labels positional required (#11529) Search refactoring 2 (#11584) Search refactoring 1 (#11572)
Arch board suggestion in #11283
Rationale for making it positional is that I feel there is an obvious logical ordering between url and the use_training_label params. Also, "required" keyword-onlys are weird