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.

Sign up for GitHub

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

Jump to bottom

Typed State #350

Merged
merged 10 commits into from
Sep 15, 2024
Merged

Typed State #350

merged 10 commits into from
Sep 15, 2024

Conversation

elijahbenizzy
Copy link
Contributor

@elijahbenizzy elijahbenizzy commented Sep 2, 2024 •
edited by skrawcz
Loading

Learnings:

  1. Builder pattern is ugly for this
  2. We have to use dual type variables (this actually makes sense) to show that with_typing_system changes it
  3. Will support both centralized and decentralized. IDE integration requires centralized, decentralized will not have it
  4. You can always add decentralized to centralized (type-hints to make your life easier), although you'll be responsible for ensuring types match, otherwise we'll want to make it error.
  5. Centralized -> decentralized is similar process

Some magic around subsetting that we have to test, see how it interacts with pydantic validation

Changes

How I tested this

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

@elijahbenizzy
Copy link
Contributor Author

elijahbenizzy commented Sep 2, 2024 •
edited
Loading

OK, TODO before this is:

Ready for feedback

  • Wire through parameters + generics to all points in application that are necessary
    • ApplicationBuilder
    • Application
    • State
    • StreamingResultContainer
  • Pydantic action
    • Implement
    • Add type-subsetting
    • Unit Tests for everything
    • Add validation configuration (from global?)
    • Change from @pydantic_action to @action.pydantic
    • Add pydantic APIs for streaming function, async streaming function, etc...
  • Run E2E
  • Unit tests
    • Pydantic state
    • Centralized state + typing
    • Streaming
  • Simple README page for examples

Ready to publish

  • with typing system
    • Comment out all typing system pieces that are not implemented yet/make them not abstract fns. Note that it is an internal API
  • Store metadata on action
    • Schema fields? E.G. information on what it has?
  • Store metadata on state as well?
    • E.G. json schema? TBD
  • Testing
    • Add tests for get inputs
    • Add E2E tests for typing
  • Docs
    • Ensure two new decorators have docstrings
    • Touch up concepts section
  • Examples
    • Add instructor example
    • Ditto, streaming
    • Checkout old youtube code
      • Code example block
      • Code example streaming
      • Server example
  • Figure out why streaming isn't showing in UI
  • Get validation of intermediate results working again for stremaing
  • Ensure we have the right code for the streaming actions
  • Add typing of state containers
  • Polish
    • Break into commits
    • All PR comments
  • Create issue with typed state features that we have not gotten around to yet

If time

  • Add type-annotations to API for action, decentralized
    • Associated tests
    • Associated docs

@elijahbenizzy elijahbenizzy force-pushed the typed-state-prototype branch 3 times, most recently from a3c981d to a6db463 Compare September 2, 2024 21:22
@elijahbenizzy elijahbenizzy changed the title WIP/scratchpad for state typing Typed State Sep 3, 2024
@elijahbenizzy elijahbenizzy marked this pull request as draft September 3, 2024 05:57
@elijahbenizzy elijahbenizzy force-pushed the typed-state-prototype branch 10 times, most recently from 35d4257 to b03b85c Compare September 10, 2024 21:24
@elijahbenizzy elijahbenizzy force-pushed the typed-state-prototype branch 2 times, most recently from d13284b to 913936a Compare September 11, 2024 22:46
elijahbenizzy
elijahbenizzy commented Sep 11, 2024
View reviewed changes
burr/core/action.py Outdated Show resolved Hide resolved
burr/core/application.py Outdated Show resolved Hide resolved
burr/core/application.py Outdated Show resolved Hide resolved
burr/core/application.py Outdated Show resolved Hide resolved
burr/core/application.py Outdated Show resolved Hide resolved
burr/integrations/pydantic.py Show resolved Hide resolved
burr/integrations/pydantic.py Show resolved Hide resolved
examples/youtube-to-social-media-post/application.py Outdated Show resolved Hide resolved
examples/youtube-to-social-media-post/application.py Outdated Show resolved Hide resolved
telemetry/ui/src/components/routes/app/StepList.tsx Outdated Show resolved Hide resolved
@elijahbenizzy elijahbenizzy force-pushed the typed-state-prototype branch 3 times, most recently from d503827 to 0c2f673 Compare September 12, 2024 04:53
@elijahbenizzy
Adds typed state support
ac0066e 
This has qwuite a few pieces to it:
1. Adds centralized state with a typing system
2. Adds decentralized state
3. Adds pydantic implementations

See issue #139 for more
details on design decisions
@elijahbenizzy
Fixes issue in which streaming actions were not expandable in the UI
361601f
@elijahbenizzy
Adds schema tracking to action
ef9906f 
This is just a Quick wrapper class to represent a schema. Note that this is currently used internally,
just to store the appropriate information. This does not validate or do conversion, currently that
is done within the pydantic model state typing system (which is also internal in its implementation).

We will likely centralize that logic at some point when we get more -- it would look something like this:
1. Action is passed an ActionSchema
2. Action is parameterized on the ActionSchema types
3. Action takes state, validates the type and converts to StateInputType
4. Action runs, returns intermediate result + state
5. Action validates intermediate result type (or converts to dict? Probably just keeps it
6. Action converts StateOutputType to State

Also we don't have this split out into two classes (input/output schema), but we will likely do that shortly.
@elijahbenizzy
Fixes up typing for streaming result containers
73dc3cf
@elijahbenizzy
Adds pydantic field to action for easy use
5bb0062 
This is just syntactic sugar so we can call @action.pydantic(...).
This does some tricky importing stuff, but does not change the required
packages in any way
@elijahbenizzy
Updates youtube server example
d9c6b59
@elijahbenizzy
Adds more tests for pydantic typed state implementation
a6705db
skrawcz
skrawcz reviewed Sep 14, 2024
View reviewed changes
examples/typed-state/README.md
)
```

Note that this should exactly model your state -- we need to make things optional,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Note that this should exactly model your state -- we need to make things optional,
Note (1): what is defined here, should exactly model all the fields required for your application to function. Note (2): we need to make fields optional in most situations,

skrawcz
skrawcz reviewed Sep 14, 2024
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
skrawcz
skrawcz approved these changes Sep 14, 2024
View reviewed changes
Copy link
Contributor

@skrawcz skrawcz left a comment •
edited by elijahbenizzy
Loading

Choose a reason for hiding this comment

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

see my edits. otherwise looks good as discussed!

@elijahbenizzy elijahbenizzy merged commit d557962 into main Sep 15, 2024
12 checks passed
@elijahbenizzy elijahbenizzy deleted the typed-state-prototype branch September 15, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skrawcz skrawcz skrawcz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@elijahbenizzy @skrawcz