[DRAFT] Pipeline Statefulness for Open World Challenges#277
Conversation
There was a problem hiding this comment.
Addes a reset_history default NOOP for backwards compatibility and so that we do not have to check hasattr in the code.
In fact I would push to do this same pattern for output_conflict_resolver to avoid the hasattr check for that too.
There was a problem hiding this comment.
Yeah I agree with you that doing the same thing with output_conflict_resolver would tidy some things up; however the way it's set up currently it's configurable at the hydra config level, but this change would only make it configurable at the code level (I think)? FWIW we don't use this pattern very often but here's one config where it's used: https://github.com/ITM-Kitware/align-system/blob/main/align_system/configs/adm/phase2_pipeline_fewshot_comparative_regression_swap_average.yaml#L53
There was a problem hiding this comment.
Cool, so in the ABC it would look like:
def output_conflict_resolver(*args, **kwargs):
return *args, **kwargs
i.e. a Callable type noop, and the hydra in your example would override it in the subclass with the injected callable.
There was a problem hiding this comment.
Hmm, can you override members like that in the hydra config? I was under the impression that you can only set / pass initialization args with hydra instantiation. If the config I linked works without modification with this ABC update you're suggesting then I'm in favor of this change.
| def reset_history(self): | ||
| self.history.clear() | ||
| for step in self.steps: | ||
| step.reset_history() No newline at end of file |
There was a problem hiding this comment.
pipeline steps may retain their own state. e.g. I am thinking about how the prompt creation step may keep a dialog history, extracting the last dialog element from history[-1]. Of course that step could also be implemented statelesly if PipelineADM.history.maxlen = None, but this gives us flexibility.
| log.info(f'[bold]*Scenario ID*[/bold]: {scenario.id()}') | ||
|
|
||
| # Reset any decision or chat history for a new scenario | ||
| if hasattr(adm, 'reset_history'): |
There was a problem hiding this comment.
No longer need this LBYL, since we guaranty the existence of this attribute
There was a problem hiding this comment.
Call it superstition, but I'd rather leave the check in there. There's a chance some older ADMs don't implement the ActionBasedADM interface (or that some newer ones don't).
There was a problem hiding this comment.
Fine with me, your call.
dmjoy
left a comment
dmjoy
left a comment
There was a problem hiding this comment.
I like this implementation (for the same reasons you state). Only change I'd like to see is to keep the LBYL for reset_history.
| log.info(f'[bold]*Scenario ID*[/bold]: {scenario.id()}') | ||
|
|
||
| # Reset any decision or chat history for a new scenario | ||
| if hasattr(adm, 'reset_history'): |
There was a problem hiding this comment.
Call it superstition, but I'd rather leave the check in there. There's a chance some older ADMs don't implement the ActionBasedADM interface (or that some newer ones don't).
There was a problem hiding this comment.
Yeah I agree with you that doing the same thing with output_conflict_resolver would tidy some things up; however the way it's set up currently it's configurable at the hydra config level, but this change would only make it configurable at the code level (I think)? FWIW we don't use this pattern very often but here's one config where it's used: https://github.com/ITM-Kitware/align-system/blob/main/align_system/configs/adm/phase2_pipeline_fewshot_comparative_regression_swap_average.yaml#L53
2 participants
No description provided.