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

Improve Support for Default Dataclass Fields #726

Merged
merged 7 commits into from
Jun 7, 2024

Conversation

HonakerM
Copy link
Contributor

What this PR does / why we need it:
This PR improves the way Caikit handles default fields in 2 main ways. The first is to always honoring dataclass default/default_factory fields while the second is improved visibility in the openapi generation.

Special notes for your reviewer:
I discovered a bug in the vectors code where the vectors assumed default value would be None instead of the list defined in the factory

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
@HonakerM HonakerM force-pushed the add_better_default_support branch from fdea0f7 to a5ae46e Compare May 29, 2024 16:43
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
@HonakerM HonakerM mentioned this pull request May 30, 2024
7 tasks
Copy link
Collaborator

@gabe-l-hart gabe-l-hart left a comment

Choose a reason for hiding this comment

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

This is some nice surgery to make the @dataclass behavior more canonical. Since it's deep in the guts, I've got a couple of places I'd like clarifying comments, but otherwise I think this will be a nice improvement!

caikit/core/data_model/base.py Show resolved Hide resolved
caikit/core/data_model/dataobject.py Outdated Show resolved Hide resolved
caikit/core/data_model/dataobject.py Outdated Show resolved Hide resolved
caikit/core/data_model/dataobject.py Show resolved Hide resolved
caikit/runtime/http_server/pydantic_wrapper.py Outdated Show resolved Hide resolved
tests/interfaces/common/test_vectors.py Show resolved Hide resolved
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
@HonakerM HonakerM requested a review from gabe-l-hart June 5, 2024 12:18
Copy link
Collaborator

@gabe-l-hart gabe-l-hart left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @HonakerM!

setattr(cls, annotation, dataclass_defined_default)
defined_default = dataclass_defined_default

if isinstance(defined_default, Callable):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

@gabe-l-hart gabe-l-hart merged commit a995e31 into caikit:main Jun 7, 2024
8 checks passed
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.

2 participants