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

Navigate Pipeline via Pipes instead of Blocks #491

Merged
merged 14 commits into from
Jan 15, 2024

Conversation

georg-schwarz
Copy link
Member

@georg-schwarz georg-schwarz commented Dec 19, 2023

Until now, we have navigated over all blocks within a pipeline and connected them via pipes to get our execution order.
However, #485 will change that by allowing block definitions outside of pipelines, raising the necessity to iterate purely over the pipes with a pipeline.

This PR introduces a PipelineWrapper in the already existing of AstNodeWrappers that inhibits the functionality to navigate over a pipeline and replaces the existing helper methods in the model-util.

The PR should not change the behavior of Jayvee besides:

  • Pipelines with one block but no pipes were previously valid, which is not the case anymore (see adapted tests).

TODOs:

  • make it work for Pipelines
  • make it work for CompositeBlockTypes (=> showcased by gtfs-static example)

@georg-schwarz georg-schwarz marked this pull request as draft December 19, 2023 15:54
Copy link
Contributor

@rhazn rhazn left a comment

Choose a reason for hiding this comment

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

I left some comments for barely relevant things outside of the scope of this PR (e.g. GTFSExtractor and I am happy if you ignore them.

I am however happy to die on the hill of using DAG naming (parents, children etc) so I'd ask for reverting the renaming to followIngoing/OutgoingPipes.

-> TripsFilePicker
-> TripsTextFileInterpreter
-> TripsCSVInterpreter
-> TripsTableInterpreter
-> TripsLoader;

// 3. As a first step, we download the zip file and interpret it.
block GTFSSampleFeedExtractor oftype HttpExtractor {
block GTFSSampleFeedExtractor oftype GTFSExtractor {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is not part of the PR but this seems very boring. We could enhance the GTFSExtractor in the std lib to also include picking a file and parsing it as CSV, maybe as a composite block inside a composite block.

The new block could be called GTFSFileExtractor and take an url and a path as property, then use the existing GTFSExtractor with the url and the FilePicker with the file, then parse that file as CSV file. This would reduce this example by 3 blocks and 3 lines for every file (so 33 less blocks and 33 less lines in the pipeline itself!).

Copy link
Member Author

Choose a reason for hiding this comment

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

If you don't mind I'd leave it as it is for now.

But your example looks interesting, also in the face of optimizations. In the best case, we would also reuse executed blocks multiple times. I'm not sure if that would be the case in the current implementation, so I'd leave it open for a follow-up issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree on all counts, it's not in the scope of this PR anyway.

libs/execution/src/lib/blocks/block-execution-util.ts Outdated Show resolved Hide resolved
@georg-schwarz georg-schwarz marked this pull request as ready for review January 12, 2024 14:44
Comment on lines 16 to 19
export class PipelineWrapper
implements AstNodeWrapper<PipelineDefinition | CompositeBlocktypeDefinition>
{
public readonly astNode: PipelineDefinition | CompositeBlocktypeDefinition;
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought about transforming it into the following so that astNode is typed better. I'm not sure if we have a benefit from that though.

Suggested change
export class PipelineWrapper
implements AstNodeWrapper<PipelineDefinition | CompositeBlocktypeDefinition>
{
public readonly astNode: PipelineDefinition | CompositeBlocktypeDefinition;
export class PipelineWrapper<
T extends PipelineDefinition | CompositeBlocktypeDefinition,
> implements AstNodeWrapper<T>
{
public readonly astNode: T;

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome, seems to work now after removing the SimplePipe syntax in #487

@georg-schwarz georg-schwarz merged commit 00867cd into main Jan 15, 2024
3 checks passed
@georg-schwarz georg-schwarz deleted the navigate-pipes-instead-blocks branch January 15, 2024 14:24
@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants