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

PipelineContext replaced with PipelineResult #2421

Open
wants to merge 2 commits into
base: main-powderhouse
Choose a base branch
from

Conversation

KathleenDollard
Copy link
Contributor

@KathleenDollard KathleenDollard commented May 6, 2024

Previously, PipelineContext was ephemeral, subsystems carried input specific caches (oops), was often recreated, and the return was CliExit.

Now, PipelineResult carries all three purposes and a single instance is used for the life of the pipeline run (per call to Execute)

This is currently in draft state because it will have merge conflicts with #2413, includes all the commits from #2413, and because it needs a cleanup/my review pass Edit: Now up to date with those PRs

@KathleenDollard KathleenDollard added the Powderhouse Work to isolate parser and features label May 6, 2024
@KathleenDollard KathleenDollard force-pushed the powderhouse-kad-rename-pipelinecontext branch from abe4191 to 6bfb62d Compare May 23, 2024 12:40
@KathleenDollard KathleenDollard marked this pull request as ready for review May 23, 2024 12:45
Copy link
Member

@mhutch mhutch left a comment

Choose a reason for hiding this comment

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

Changes look ok. I do wonder whether we might want the pipeline to return a result type that wraps the pipeline state object, is immutable and doesn't have all the members available to the subsystems. In that case the name PipelineResult might be better reserved for that wrapper type. But that would be easy to change anytime before we freeze the API, so let's merge the PR as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Powderhouse Work to isolate parser and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants