Skip to content

Nested pydantic model containers #669

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ndxmrb
Copy link

@ndxmrb ndxmrb commented Sep 6, 2024

Hi @tlambert03,
when using nested pydantic models, I found this post on how to add a simple way of nesting widgets/containers. When trying to use the values, I needed to modify asdict() as well. Although there might be a reason you didn't just implement it already when answering the post (probably due to #317), I thought I'd make a pull request just in case.

Notably, some things are missing:

Also, I tried my best with the types, but I probably don't oversee the grand scheme of things... 😅

I hope it's helpful, but I'm aware it might not be, so feel free to just close 😄

Thanks for having a look!

@ndxmrb
Copy link
Author

ndxmrb commented Sep 6, 2024

I just saw that this probably messes with List types. Set to draft for now.

Copy link

codecov bot commented Sep 10, 2024

Codecov Report

Attention: Patch coverage is 57.89474% with 8 lines in your changes missing coverage. Please review.

Project coverage is 89.03%. Comparing base (379bc62) to head (15263c6).

Files with missing lines Patch % Lines
src/magicgui/schema/_ui_field.py 36.36% 7 Missing ⚠️
src/magicgui/widgets/bases/_container_widget.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #669      +/-   ##
==========================================
- Coverage   89.16%   89.03%   -0.14%     
==========================================
  Files          39       39              
  Lines        4746     4761      +15     
==========================================
+ Hits         4232     4239       +7     
- Misses        514      522       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tlambert03
Copy link
Member

sorry for the radio silence here @ndxmrb. This would be a great addition if you want to keep banging at it.

I did indeed see the challenge with list types. I wonder whether #663 might have helped the situation here (I haven't had time to look closely, so it's possibly no). @hanjinliu may also have some insights...

if you have a moment and want to resolve merge conflicts and see what the state of things look like now, that would be great :)

@ndxmrb
Copy link
Author

ndxmrb commented Apr 16, 2025

Sorry for the silence as well @tlambert03. I'm still willing to work on it. I think the latest work here has indeed helped with the list types. But unfortunately, I'm not sure anymore what problem exactly I encountered. Either way, I can now use a nested pydantic model alongside a list field and validate the resulting dict without any issues.

Do you think the NestedValueWidgets type is still necessary, now that containers subclass ValueWidgets?

I'll update this (or make a new PR, if that's easier) as soon as I know how to go on with that.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants