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

Pipeline viz #1

Merged
merged 29 commits into from
Jan 16, 2024
Merged

Pipeline viz #1

merged 29 commits into from
Jan 16, 2024

Conversation

fabalex7
Copy link
Owner

merging pipeline_viz (with cli support) into main

Copy link

@joluj joluj left a comment

Choose a reason for hiding this comment

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

General comments:

  • use kebab-case instead of snake_case for file names (we do that everywhere else, so just for consistency).
  • use camelCase for variables, constants and functions.
  • refactor the code to work with the AST functions instead of the runtime types. This makes the code much more solid and more errors will be detected on runtime. Also, it then should be a bit more readable. I commented an excerpt of important functions for you, you'll probably don't need much more.

setMermaidTheme,
} from './mermaid_utils';

export async function myRunAction(
Copy link

Choose a reason for hiding this comment

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

Use proper names


const model = await extractAstNodeFn(services, loggerFactory);

const myblocks = getBlocksInTopologicalSorting(model.pipelines[0]!)
Copy link

Choose a reason for hiding this comment

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

This will fail if there a no pipelines in the model and it will result in non-intuitive behavior if there are more than one pipelines defined.

Both cases do not make sense at the current state of Jayvee, but I'd prefer some sanity check there (i.e. assert that there is exactly one pipeline and fail otherwise).

fileName: string,
mermaidOptions: MermaidOptions
){
console.log('Hello World!');
Copy link

Choose a reason for hiding this comment

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

Suggested change
console.log('Hello World!');

debugTarget: undefined
}
const result = await myRunAction(fileName, options, mermaidOptions);
console.log("End World");
Copy link

Choose a reason for hiding this comment

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

Suggested change
console.log("End World");

return ExitCode.SUCCESS;
}

async function myCall(
Copy link

Choose a reason for hiding this comment

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

Naming

if (mermaidOptions.properties){
for (let property of block.body.properties) {
if (properties.includes(property.name)){
let pv: any = property.value // Is there a better way of doing this?
Copy link

Choose a reason for hiding this comment

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

// Is there a better way of doing this?

Yes, using the AST :D

Comment on lines 115 to 120
if (block.name.includes("Extractor")){
classAssign.push(`class ${block.name} source;`)
}
if (block.name.includes("Loader")){
classAssign.push(`class ${block.name} sink;`)
}
Copy link

Choose a reason for hiding this comment

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

The block names are arbitrary, i.e. it is possible to name an extractor MyCoolLoader. You should use our utility functions (in this case the attribute inputType of a BlockTypeWrapper could help)

example/cars2.jv Outdated
@@ -0,0 +1,221 @@
// SPDX-FileCopyrightText: 2023 Friedrich-Alexander-Universitat Erlangen-Nurnberg
Copy link

Choose a reason for hiding this comment

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

I'd remove this file from git.

@@ -0,0 +1,45 @@
pipeline AirportsPipeline {
Copy link

Choose a reason for hiding this comment

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

What benefit does this file bring?

@@ -0,0 +1,83 @@
// Define a new blocktype named CSVExtractor outside of the pipeline
Copy link

Choose a reason for hiding this comment

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

Nice example of composite blocktypes. I'd be happy if we can use this example later, but it needs some polishing first (and renaming)

@fabalex7
Copy link
Owner Author

included changes, please review again @joluj

Copy link

@joluj joluj left a comment

Choose a reason for hiding this comment

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

Commented instead of approved since the Actions did not run. They will run when you push the next time.

Comment on lines 47 to 48
const listofPipes: Array<string[]> = [];
const listofBocks: Array<string> = [];
Copy link

Choose a reason for hiding this comment

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

Suggested change
const listofPipes: Array<string[]> = [];
const listofBocks: Array<string> = [];
const listOfPipes: Array<string[]> = [];
const listOfBocks: Array<string> = [];

if (mermaidOptions.properties) {
for (const property of block.body.properties) {
if (properties.includes(property.name)) {
if (isTextLiteral(property.value)) {
Copy link

Choose a reason for hiding this comment

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

Keep in mind that there are other literals that are also easy and important to display, for example numeric or boolean literals. Can be done later though.

@@ -0,0 +1,83 @@
// Define a new blocktype named CSVExtractor outside of the pipeline
Copy link

Choose a reason for hiding this comment

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

If the file is in the example folder it will be published as in the Jayvee documentation (https://jvalue.github.io/jayvee/docs/category/jayvee-examples).

Since this file is only relevant for testing purposes, I'd suggest moving it to apps/pipeline-visualization/test/assets/nested-composite-blocks.jv (create folders and rename).

@joluj
Copy link

joluj commented Jan 15, 2024

The CI Pipeline fails because of missing licenses. In our project, every file must have a valid license.

For files that allow comments: Add them at the top (Example for Jayvee Files and TypeScript files).
For files that don't allow them: Create .license-file (Example directory with project.json and project.json.license).

The CLA Assistant fails which we can ignore in a fork.

@fabalex7 fabalex7 merged commit fb74ff1 into main Jan 16, 2024
1 of 2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants