-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 combiner schema validation #1347
Conversation
…/ludwig into add_combiner_schema_validation
…/ludwig into add_combiner_schema_validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work on this! I especially like all the tests.
Looks like a few tests are still failing in CI. Can you take a look?
ludwig/combiners/combiners.py
Outdated
return SequenceCombinerParams | ||
|
||
|
||
class TabNetCombinerParams(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably err on the side of being more permissive here, since I don't know offhand what the valid ranges are for all these.
I would say we should make the following changes:
- relaxation_factor: NonNegativeFloat
- bn_epsilon: float
- bn_momentum: float
- sparsity: float
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed.
conds.append(combiner_cond) | ||
return conds | ||
|
||
def get_custom_definitions(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting. What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pulls out any $refs (e.g. for the enums) from each combiner's schema and moves it to a root-level 'definitions' sub-schema. This way there is less nested stuff and the logic/schemas are cleaner overall. But we can move it to a different part of the full schema if need be.
@ksbrar, wish you had mentioned this concern earlier, I could have saved you some time ;) https://pypi.org/project/marshmallow-dataclass/ Basically, there's a package we can use to combine the schema and the data into a single object. |
…cause initializers are unions that accept both dicts and strings they have trouble with the dataclass constructors - looking at combiners.py, you can see all other fields are converted and working as expected.
The general approach here is as follows:
For this PR, we're only focusing on combiners, but the idea is to expand this to other parts of the config over time.