-
Notifications
You must be signed in to change notification settings - Fork 75
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
Comments
OK, implementation in #269. Small caveat, however:
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. |
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. |
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. Otherwise both should be testable conditions if you really need to check for it. |
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:
Currently this would break in both cases with the following 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?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.
The text was updated successfully, but these errors were encountered: