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

Create support for a new Dataclass Transform #266

Closed
wants to merge 2 commits into from

Conversation

gedemagt
Copy link

This PR adds a new Transform that transforms parameters to dataclasses, very inspired by the way FastAPI works with pydantic.

It is mainly useful for states, but allows one to write something like the following and get syntax-highlighting and automatic serialization/deserialization:

@dataclass
class StateModel:
    value: datetime
    list_of_values: list[int]

@callback(
    Output('output-div','children'),
    Trigger('some-btn', 'n_clicks'),
    State('state-object', 'data')
)
def do_stuff(state: StateModel):
    return f"Before now? {state.value > datetime.utcnow()} - with {len(state.list_of_values)} values"

It uses the dataclass-wizard library to support out-of-box support for data types such as datetime as well as nested models, which is not supported by the standard library fromdict method.

@codecov-commenter
Copy link

Codecov Report

Merging #266 (3c24de2) into master (2bf9f45) will decrease coverage by 2.22%.
The diff coverage is 28.20%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
- Coverage   84.12%   81.91%   -2.22%     
==========================================
  Files           9        9              
  Lines         945      984      +39     
==========================================
+ Hits          795      806      +11     
- Misses        150      178      +28     
Impacted Files Coverage Δ
dash_extensions/enrich.py 79.50% <28.20%> (-2.60%) ⬇️

@gedemagt gedemagt requested a review from emilhe July 2, 2023 07:52
@emilhe
Copy link
Owner

emilhe commented Jul 2, 2023

I really like this concept! However, it would need some tests before the PR can be merged. Also, it is not clear to me, if the current implementation will support flexible callback signatures?

Finally, I was thinking that it might be an options to separate out common functionality related to inputs/output packing between this transform and the serverside output transform (including the massaging needed to support flexible callback signatures). At at glance, they seem to have very much in common (apart from the transformation itself). I'll try to make a draft, when I find the time.

@gedemagt
Copy link
Author

gedemagt commented Jul 2, 2023

Yes, tests are indeed needed - wanted to check the concept before adding them :)

As for flexible signatures, I believe it should work - but I will test a bit. SInce the transform is only based on the type-hint in the function, it will transform whatever the rest of the dash framework (and extensions) will provide as arguments. That does mean that the user potentially needs to make a model that both includes the data structure of the content and also the input/state signature. I think that is okay, since it keeps the complexity.

I did consider whether you should tie the models directly to the Store, but I think this is much more flexible.

As for the last point - not sure I understand, but looking forward to see the draft :D

@emilhe
Copy link
Owner

emilhe commented Jul 2, 2023

@gedemagt as for the last point, I was thinking something like this where the general logic related to "doing custom serialization" is reused across the different implementations (e.g. data classes and serverside caching). What do you think about this approach?

@gedemagt
Copy link
Author

gedemagt commented Jul 6, 2023

I see - makes sense. Simplifies quite a bit. Will try to test this also.

@emilhe
Copy link
Owner

emilhe commented Jul 31, 2023

The concept was merged in #271

@emilhe emilhe closed this Jul 31, 2023
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

3 participants