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

fix: Raise error if list provided to StringOptions has duplicates and change validation errors to assertion errors for this field as well. #3302

Merged
merged 4 commits into from
Mar 30, 2023

Conversation

ksbrar
Copy link
Collaborator

@ksbrar ksbrar commented Mar 29, 2023

No description provided.

@tgaddair
Copy link
Collaborator

@ksbrar were you going to change these to asserts?

@ksbrar
Copy link
Collaborator Author

ksbrar commented Mar 29, 2023

@tgaddair yes doing it now, was fixing other things

@github-actions
Copy link

github-actions bot commented Mar 29, 2023

Unit Test Results

       6 files  ±       0         6 suites  ±0   59m 13s ⏱️ + 37m 2s
2 485 tests +2 473  2 475 ✔️ +2 465    9 💤 +  7  1 +1 
7 455 runs  +7 395  7 425 ✔️ +7 377  27 💤 +15  3 +3 

For more details on these failures, see this check.

Results for commit dd59940. ± Comparison against base commit 531e024.

This pull request removes 4 and adds 2477 tests. Note that renamed tests count towards both.
tests.regression_tests.model.test_old_models ‑ test_model_loaded_from_old_config_prediction_works
tests.regression_tests.model.test_old_models ‑ test_predict_deprecated_model[respiratory]
tests.regression_tests.model.test_old_models ‑ test_predict_deprecated_model[titanic]
tests.regression_tests.model.test_old_models ‑ test_predict_deprecated_model[twitter_bots]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_image_augmentation[augmentation_pipeline_ops0]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_image_augmentation[augmentation_pipeline_ops1]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_image_augmentation[augmentation_pipeline_ops2]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_invalid_augmentation_parameters[None]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_invalid_augmentation_parameters[augmentation_pipeline_ops1]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_invalid_augmentation_parameters[augmentation_pipeline_ops2]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_invalid_augmentation_parameters[augmentation_pipeline_ops4]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_invalid_augmentation_parameters[random_horizontal_flip]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_load_model_with_augmentation_pipeline
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_local_model_training_with_augmentation_pipeline[preprocessing0-encoder0-False]
…

♻️ This comment has been updated with latest results.

@ksbrar ksbrar changed the title fix: Raise error if list provided to StringOptions has duplicates fix: Raise error if list provided to StringOptions has duplicates and change validation errors to assertion errors for this field as well. Mar 30, 2023
@ksbrar ksbrar requested a review from tgaddair March 30, 2023 02:03
@ksbrar
Copy link
Collaborator Author

ksbrar commented Mar 30, 2023

@tgaddair Switched to assertions, please re-review if you would like.

@ksbrar ksbrar merged commit ca2c859 into master Mar 30, 2023
@ksbrar ksbrar deleted the string-options-uniqueness branch March 30, 2023 17:52
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

4 participants