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

Default reads/writes in actions #268

Open
elijahbenizzy opened this issue Jul 11, 2024 · 5 comments
Open

Default reads/writes in actions #268

elijahbenizzy opened this issue Jul 11, 2024 · 5 comments
Labels
design decision enhancement New feature or request

Comments

@elijahbenizzy
Copy link
Contributor

elijahbenizzy commented Jul 11, 2024

Is your feature request related to a problem? Please describe.

There are cases in which you only want to write one thing to state. Take, for example, this:

@action(reads=["prompt"], writes=["response", "error"])
def respond(state: State) -> State:
    try:
        return state.update(response=_query(state["prompt"]))
    except SpecificException as e:
        return state.update(error=e)

Currently this would break in both cases with the following error:

File [~/dev/dagworks/os/burr/burr/core/application.py:155](http://localhost:8888/lab/workspaces/auto-3/tree/~/dev/dagworks/os/burr/burr/core/application.py#line=154), in _validate_reducer_writes(reducer, state, name)
    153 missing_writes = set(reducer.writes) - state.keys()
    154 if len(missing_writes) > 0:
--> 155     raise ValueError(
    156         f"State is missing write keys after running: {name}. Missing keys are: {missing_writes}. "
    157         f"Has writes: {required_writes}"
    158     )

ValueError: State is missing write keys after running: respond. Missing keys are: {'response'}. Has writes: ['response', 'error']`

Describe the solution you'd like
In most cases, people will end up padding with None, or some other sentinel value. What if we made it a specific exception?

@action(reads=["prompt"], writes=["response", "error"], default_writes={"response" : None, "error" : None})
def respond(state: State) -> State:
    try:
        return state.update(response=_query(state["prompt"]))
    except SpecificException as e:
        return state.update(error=e)

Describe alternatives you've considered
We could leverage the typing system with Optional see #139, or not enable this. That said, I think this is a fairly simple pattern.

Other Q is how to specify that just one of these is required. Tooling like pydantic requires custom validation for this, which feels a little overboard... Defaults is a nice but clean way to do this I think.

Additional context

Based on this discord question: https://discord.com/channels/1221891403253288970/1221892061842772018/1260680704002494514.

@elijahbenizzy elijahbenizzy added enhancement New feature or request design decision labels Jul 11, 2024
@elijahbenizzy
Copy link
Contributor Author

elijahbenizzy commented Jul 16, 2024

OK, implementation in #269. Small caveat, however:

  1. In the multi-step process (E.G. those that define reduce/run separately) -- it will apply the default if it already exists in state and the action does not specify it
  2. In the single-step process (E.G. the function-based API) -- it will apply the deafult only if it does not already exist in state and the action does not specify it.

This is because we need the state reads and writes in the same object, so we can't just apply the default to the non-object. I think that the behavior in (1) is actually much cleaner. I'm going to see if I can add a quick state log to tell the writes in a time-period, otherwise probably put this on hold for a bit and think through.

#269 proves out that we can do this with recording the state writes. Note this is also a step towards #33. I did this more to explore, will be mulling over tonight then deciding the way I want to implement this.

@skrawcz
Copy link
Contributor

skrawcz commented Jul 17, 2024

I'm not sure we really need this, because one can just do this at the beginning -- and reading the code it's clear:

@action(reads=["prompt"], writes=["response", "error"])
def respond(state: State) -> State:
    state = state.update(response=None, error=None) # set defaults 
    try:
        return state.update(response=_query(state["prompt"]))
    except SpecificException as e:
        return state.update(error=e)

This below, would just be syntactic sugar for the above IMO - i.e. it would apply it to the beginning:

@action(reads=["prompt"], writes=["response", "error"], default_writes={"response" : None, "error" : None})
def respond(state: State) -> State:
    try:
        return state.update(response=_query(state["prompt"]))
    except SpecificException as e:
        return state.update(error=e)

@elijahbenizzy
Copy link
Contributor Author

I'm not sure we really need this, because one can just do this at the beginning -- and reading the code it's clear:

@action(reads=["prompt"], writes=["response", "error"])

def respond(state: State) -> State:

    state = state.update(response=None, error=None) # set defaults 

    try:

        return state.update(response=_query(state["prompt"]))

    except SpecificException as e:

        return state.update(error=e)

This below, would just be syntactic sugar for the above IMO - i.e. it would apply it to the beginning:

@action(reads=["prompt"], writes=["response", "error"], default_writes={"response" : None, "error" : None})

def respond(state: State) -> State:

    try:

        return state.update(response=_query(state["prompt"]))

    except SpecificException as e:

        return state.update(error=e)

I think it's nice to have to simplify -- a state update at the beginning can be a little error-prone. But yeah, that's a much better way to implement, nice.

@ibhandari
Copy link

I agree with @elijahbenizzy. Updating a default state in the beginning of every action can be error prone especially if you have multiple conditions where state changes significantly or when the state has non-null variable (our case).

@skrawcz
Copy link
Contributor

skrawcz commented Jul 18, 2024

I agree with @elijahbenizzy. Updating a default state in the beginning of every action can be error prone especially if you have multiple conditions where state changes significantly or when the state has non-null variable (our case).

Not sure I follow @ibhandari. Could you expand on that a bit more?

For example, you could just as easily forget to add the default value in the decorator.
So I see providing default values at the beginning, or via the decorator to be isomorphic; the end result is the same. It's just where you put that code.

Otherwise both should be testable conditions if you really need to check for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design decision enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants