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

Populate generated form values from a pydantic model instance #16

Merged
merged 15 commits into from Jul 19, 2022

Conversation

HIL340
Copy link
Contributor

@HIL340 HIL340 commented Jul 14, 2022

What kind of change does this PR introduce?

  • Bugfix
  • New Feature
  • Feature Improvement
  • Refactoring
  • Documentation
  • Other, please describe:

Description:

This PR adds the capability to provide a pydantic model instance to sp.pydantic_input() and have the generated pydantic widgets be pre-populated with the instance property values.

This allows for full CRUD operations to be performed on pydantic data using streamlit.

I've been tinkering with this and some other changes in my fork and have made good use of it in a couple of projects now and it would be nice to get it merged upstream.

To demonstrate the functionality I've added two additional examples to the playground:

  • complex_instance_model.py: A full demo that shows all of the major widget types and the difference in behaviour between an empty pydantic schema vs a fully populated instance
  • union_field_instance.py: An example to demonstrate a union field using an instance with Discriminated Unions

Checklist:

  • I have read the CONTRIBUTING document.
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

@HIL340 HIL340 requested a review from LukasMasuch as a code owner July 14, 2022 14:40
@HIL340
Copy link
Contributor Author

HIL340 commented Jul 14, 2022

Closes #15

Copy link
Owner

@LukasMasuch LukasMasuch left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this awesome contribution 👍 Implemention looks good and everything in complex_instance_model works fine. However, there seems to be a problem with the union_field_instance example.

src/streamlit_pydantic/ui_renderer.py Outdated Show resolved Hide resolved
src/streamlit_pydantic/ui_renderer.py Show resolved Hide resolved
@LukasMasuch
Copy link
Owner

LukasMasuch commented Jul 16, 2022

Ahh, I think I figured out why it is not working. Discriminated unions are only supported since pydantic 1.9. So, we might be fine if we just pin pydantic to >=1.9 in setup.py.

@HIL340
Copy link
Contributor Author

HIL340 commented Jul 17, 2022

Thanks for the suggestions!

In regards to performance, note there there can sometimes be some noticeable lag/jitters when interacting with some of the list value widgets, particularly when the model is large and/or there is multiple models on screen.

This is pretty well demonstrated in complex_instance_model where there are two large complex objects (one for the model and one for the instance) being rendered at the same time.

Clicking the +/- buttons on one of the number widgets within a list (eg. int list, int dict, object list) often results in a noticeable redraw and second or third button clicks are not always honoured in the widgets value.

With the complex_instance_model example this issue seems to be much more noticeable for the second of the two complex objects (regardless of which one is drawn first) and becomes much less noticeable if only one of the objects is being rendered.

I'm not sure if this is an existing issue but the new method of handling lists that is introduced with this PR (where every item is being individually rendered) is possibly making it more noticeable.

@HIL340 HIL340 requested a review from LukasMasuch July 17, 2022 09:11
@HIL340 HIL340 changed the title Populate generated form values with a pydantic model instance Populate generated form values from a pydantic model instance Jul 17, 2022
@LukasMasuch
Copy link
Owner

LukasMasuch commented Jul 17, 2022

some noticeable lag/jitters when interacting with some of the list value widgets
yes, indeed there seems to be some strange behavior :( The handling of list/dict input is something I want to replace anyways. The reason is that editable data frames will likely be implemented in Streamlit soon and this would be way better as an input widget than the current way dict/list support is implemented. So, potentially we could also keep out the instance support for list/dict types from this PR and add it once dict/list handling is replaced with editable dataframes?

Another aspect I was just thinking about: The union type instance support works great if the discriminator is set, but doesn't fill out the widgets if it is not used. Wouldn't it be possible also to try to match the correct model based on the type of the object inside the instance?

session value of a widget to initialise its value
(if it already exists in the session).
This resolves most of the value lag/bounce that can
occur with this widget when using the +/- buttons
This simplifies the rendering and resolves much of
the UI lag/jerkiness
without the need for discriminator property
Differentiate between the two union examples
@HIL340
Copy link
Contributor Author

HIL340 commented Jul 18, 2022

ok, good news. I believe I've resolved the major lag/jitter issues with the latest commits and ready for another review.

Editable data frames would indeed be a great alternative for editing lists/dicts although I imagine it would depend a bit on the data that is being represented.

The method included in this PR (now that it works a bit better) might still be preferable for smaller lists (of say 1-5 values) and I was thinking that streamlit's new tabs feature might be a really good option for rendering lists of complex objects (which can individually take up a lot of screen real estate).

With a few decent options to choose from the user could decide which method works best using the "format" Field parameter.

Also, Union type instances also now work with or without a discriminator field.

@LukasMasuch
Copy link
Owner

LukasMasuch commented Jul 18, 2022

Awesome :) It seems to work a lot better now! And also really like that Union works without discriminator field as well. I only have one last comment which might need to be addressed before merging (see above).

streamlit's new tabs feature might be a really good option for rendering lists of complex objects

I like that idea :) Indeed, tabs could simplify the UI a lot for complex object lists. And adding a configuration option for how certain properties are rendered (tabs, dataframe, ...) could be a useful addition!

make a note about aliases.  To make aliases work
here I think will require some more signficant changes
rendering objects in the sidebar
@HIL340
Copy link
Contributor Author

HIL340 commented Jul 19, 2022

In addition to the fix above I've also cleaned up the list control rendering so its usable within the sidebar.

@HIL340 HIL340 requested a review from LukasMasuch July 19, 2022 02:34
@LukasMasuch LukasMasuch merged commit ca5d3ec into LukasMasuch:main Jul 19, 2022
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

2 participants