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

Consolidate hyperopt schema in ludwig.schema.hyperopt #3190

Merged
merged 95 commits into from
Apr 5, 2023

Conversation

jeffkinnison
Copy link
Contributor

The hyperopt search algorithm schemas are defined in ludwig.hyperopt, and they currently work with dictionary schemas rather than dataclasses. This update will move all hyperopt schema into ludwig.schema.hyperopt and update hyperopt to work with object schemas.

@jeffkinnison jeffkinnison changed the title Consolidate hyperopt schema in ludwig.schema.hyperopt [Draft] Consolidate hyperopt schema in ludwig.schema.hyperopt Mar 2, 2023
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

Unit Test Results

  6 files  ±0    6 suites  ±0   22m 28s ⏱️ + 1m 52s
12 tests ±0  10 ✔️ ±0    2 💤 ±0  0 ±0 
60 runs  ±0  48 ✔️ ±0  12 💤 ±0  0 ±0 

Results for commit 98b800a. ± Comparison against base commit 3246acd.

♻️ This comment has been updated with latest results.

@jeffkinnison
Copy link
Contributor Author

The latest failures required a rewrite of the nested parameter check because of the case where we were hyperopting over specific combiner configs, and checking against the defaults led to a validation error when combiner.type changed. Because of the complexity of these checks, going forward we should be mindful to add hyperopt validation check failures to the unit tests as they come up.

ludwig/config_validation/checks.py Show resolved Hide resolved
field_type = fields.Float()
elif list_type is list:
field_type = fields.List(fields.Float())
type_map = {str: fields.String, int: fields.Integer, float: fields.Float}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👌

("out_feature.foo.num_fc_layers", True),
],
)
def test_parameter_key_check(referenced_parameter, raises_exception):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👌

else:
config.__getattribute__(k)
except AttributeError:
invalid_fields.append(nested_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is awesome

ludwig/config_validation/checks.py Show resolved Hide resolved
ludwig/config_validation/checks.py Show resolved Hide resolved
@jeffkinnison
Copy link
Contributor Author

Looks like the tagger decoder failures were happening because the check was de-registered at some point. That can happen when merging checks.py because the registry line is shared between all checks, and so not duplicated in a merge diff. Added it back in here.

Copy link
Collaborator

@justinxzhao justinxzhao left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@ksbrar ksbrar left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@arnavgarg1
Copy link
Contributor

Amazing work 👏

Copy link
Collaborator

@tgaddair tgaddair left a comment

Choose a reason for hiding this comment

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

LGTM! A lot of great work in this PR :)

@jeffkinnison jeffkinnison merged commit e49eae9 into master Apr 5, 2023
10 checks passed
@jeffkinnison jeffkinnison deleted the consolidate-hyperopt-schema branch April 5, 2023 23:30
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

5 participants