Belated Expression Workflow Changes by mfleader · Pull Request #27 · arcalot/arcaflow-workflows

mfleader · 2024-05-03T18:37:37Z

Changes introduced with this PR

Make changes that converge the expressions example towards the other examples.

By contributing to this repository, I agree to the contribution guidelines.

dbutenhof

Like Webb, I'm not comfortable with the specific container label. This implies an outstanding task of making sure we formally track references like this and keep them up-to-date...

dustinblack · 2024-05-08T09:09:59Z

There are several problems with this example workflow, most of which apply to the original commit rather than this PR's changes. I've made notes inline, but as an additional point I think the README needs to be expanded. I know there is a link to the proper documentation for expressions there, but example workflows, especially the basic ones, should provide self-contained knowledge for the new user. It is completely unclear in the README how I use this feature.

basic-examples/expressions/subworkflow.yaml

dustinblack · 2024-05-08T09:06:47Z

basic-examples/expressions/README.md

+steps.example.cancelled-->steps.example.crashed
+steps.example.crashed-->steps.example.crashed.error
+%% Mermaid end
+```


This is the mermaid chart of only the subworkflow. Since we are using a foreach here, it may make sense to include the charts for both the inner and outer workflows.

It's troublesome that both the parent and the subworkflow use the step name example, and I really think that should be changed.

added the parent workflow

mfleader · 2024-05-08T15:51:20Z

There are several problems with this example workflow, most of which apply to the original commit rather than this PR's changes. I've made notes inline, but as an additional point I think the README needs to be expanded. I know there is a link to the proper documentation for expressions there, but example workflows, especially the basic ones, should provide self-contained knowledge for the new user. It is completely unclear in the README how I use this feature.

@jaredoconnell I could use some input on what you'd want to see in the expressions example workflow readme.

mfleader · 2024-05-09T17:45:57Z

There are several problems with this example workflow, most of which apply to the original commit rather than this PR's changes. I've made notes inline, but as an additional point I think the README needs to be expanded. I know there is a link to the proper documentation for expressions there, but example workflows, especially the basic ones, should provide self-contained knowledge for the new user. It is completely unclear in the README how I use this feature.

I pulled some of the relevant linked documentation into the workflow's README.

* update workflow with clearer examples and no sub-workflow * addressing PR feedback * updates from PR feedback

dustinblack · 2024-05-17T08:40:15Z

@mfleader I heavily re-worked this myself, and those changes are merged now, so it will be a little self-serving for me to approve the PR at this point. I'd like for you to take another pass at it and make sure we've really covered the feature well now.

…ows into expression-fixes

@dustinblack

changes were effectively approved with the merge of @dustinblack's pull request #29

jaredoconnell · 2024-05-20T15:37:11Z

We would like to keep the existing example, but we will rename it to "quoting". Then this PR will be updated as a new example.

webbnh · 2024-05-20T22:30:43Z

We would like to keep the existing example, but we will rename it to "quoting". Then this PR will be updated as a new example.

This work was completed in PR #30.

Now the branch for this PR needs to be brought up to date with main, which might be slightly challenging, as Git will probably regard the upstream change as deleting the files and the downstream change as modifying them. But, if all else fails, you can just present the downstream files as "new" files (i.e., save them aside, do the pull, and then move them back and add them). (Please do not delete and recreate the branch with the same name -- GitHub tends not to handle that well.)

webbnh

So, I ran this workflow, and the output doesn't look right:

$ ../../../arcaflow -input input.yaml -config config.yaml -context . 2>&1 | sed -E -e '/^2024/ d' -e '/%% Mermaid/,/%% Mermaid/ d'
workflow.yaml:1:16: invalid char escape
workflow.yaml:1:16: invalid char escape
workflow.yaml:1:16: invalid char literal
output_data:
    message:
        message: |-
            Hello, !expr intToString(floatToInt(intToFloat($.input.int_value) *  $.input.float_value)) + ' User'
            !
    name:
        message: Hello, Arcaflow -- "The Noble Workflow Engine"!
output_id: success

(I took the liberty of filtering out the Mermaid output and the log messages.)

I'm not sure what the three "invalid char" messages are referring to, but, interestingly, they don't stop the run from starting.

And, more interesting than that, the expression in step example2 is being passed through as a literal (including its trailing newline) instead of being evaluated (perhaps as a result of the invalid characters?).

Although, the debug output does include the line,

2024-05-21T17:23:29Z	debug	source=executor	Evaluating expression $.input.name + ' -- \"The Noble Workflow Engine\"'...

so, apparently Arca knows that it's an expression.

If I put the whole expression on the same line as the property tag, then it seems to work as expected, although it still complains about the invalid char (in fact it adds a complaint).

One other curious thing to note: the file seems to be using CR/LF pairs as the line endings. I don't know if that's a problem (or if it's some sort of local setting I've got), but, when the going gets weird, I start looking at everything....

webbnh · 2024-05-21T16:47:35Z

basic-examples/expressions/workflow.yaml

-      - name: !expr '"Here''s an apostrophe and \"embedded quotes\"."'
-      - name: !expr "'Here\\'s an apostrophe and \"embedded quotes\".'"
-      - name: !expr |-
-          'Here\'s an apostrophe and "embedded quotes".'
-      - name: !expr |-
-          "Here's an apostrophe and \"embedded quotes\"."
-      - name: !expr '`Here''s an apostrophe and "embedded quotes".`'
-      - name: !expr |-
-          `Here's an apostrophe and "embedded quotes".`
+  example1:
+    plugin:
+      deployment_type: image
+      src: quay.io/arcalot/arcaflow-plugin-template-python:0.4.0
+    input:
+      #Expression with concatenation and string literal
+      name: !expr $.input.name + ' -- \"The Noble Workflow Engine\"'


Could someone remind me why we need to escape the double quotes inside a string marked with single quotes? That is, the escapes aren't actually needed, right?

(I guess that \" gets replaced with " so we get the right result, but if we want to demonstrate the application of escape characters when they are actually needed, perhaps the string should be marked with double quotes?)

The escaped double quotes are generating a warning (which doesn't seem to adversely affect the operation of the workflow). There also seems to be a warning (which, again, doesn't seem to have any adverse effect) on the use of the single quotes.

If you replace the single quotes with double quotes, it makes both sets of warnings go away (and, it justifies the escapes...).

webbnh · 2024-05-21T16:56:55Z

basic-examples/expressions/README.md

+documentation](https://arcalot.io/arcaflow/workflows/expressions/). Arcaflow 
+expressions are a way to reference paths within the workflow or their fields, and they 
+additionally allow you to manipulate the data.


The phrase "to reference paths" doesn't seem like it will mean anything to a user. That is, expressions allow a user to reference a piece of data via a path, but the path is a means to an end, not a goal in itself. And, the path should pretty much always end at a field (or a list item). So, I propose rewording this to something like,

Arcaflow expressions provide a way to reference individual data items within the workflow, such as lists, objects, list elements, and object properties, for use in specifying inputs to steps and subworkflows or workflow outputs. In addition, expressions can be used to transform input and output items to produce new data items.

webbnh · 2024-05-21T16:58:43Z

basic-examples/expressions/README.md

+*Note: Arcaflow expressions were inspired by JSONPath, and may look familar, but 
+expressions have diverged from the JSONPath syntax to enable a rich feature set that is 
+specific to Arcaflow.* 


Perhaps,

Arcaflow expression syntax was inspired by JSONPath, and may look familiar, but
the syntax has diverged from the JSONPath syntax to enable a rich feature set that is
specific to Arcaflow.

(Note the correction of the spelling of "familiar".)

webbnh · 2024-05-21T17:02:16Z

basic-examples/expressions/README.md

+## Examples
+
+Basic path references are already used across the other example workflows. These 
+include references in a step to an input from the schema:


The comment about path references already being used elsewhere is non-productive (unless we expect that the reader has already seen them or is just about to see them). Instead, let's just say what we want to say, here:

A basic path reference can be used to specify a value from the workload input as the input to a step:

webbnh · 2024-05-21T17:03:46Z

basic-examples/expressions/README.md

+    input:
+      #Expression with concatenation and string literal
+      name: !expr $.input.name + ' -- \"The Noble Workflow Engine\"'


See my comment to basic-examples/expressions/workflow.yaml.

webbnh · 2024-05-21T17:04:44Z

basic-examples/expressions/README.md

+
+As well as type conversions and math operations:


This comment refers to a new YAML blob, so replace "As well as" with something like "This workflow demonstrates".

webbnh · 2024-05-21T17:07:58Z

basic-examples/expressions/README.md

+      name: >
+        !expr intToString(floatToInt(intToFloat($.input.int_value) * 
+        $.input.float_value)) + ' User'
+```
+
+In this last expression, the plugin requires a string input. The logical steps here are:


Since this text refers to the preceding text, unlike all the previous text blobs which have each been referring to examples which follow them, it would be good to explicitly establish the context first, by inserting a phrase like "in the example above" before the comma.

webbnh · 2024-05-21T18:19:53Z

I'm not sure what the three "invalid char" messages are referring to

I got rid of the complaints about the escapes by removing the backslashes from in front of the double quotes in the string....

webbnh · 2024-05-21T18:21:10Z

I'm not sure what the three "invalid char" messages are referring to

I got rid of the complaints about the escapes by removing the backslashes from in front of the double quotes in the string....

Leaving the escapes in place and changing the single quotes to double quotes also addresses the complaints....

webbnh · 2024-05-21T18:30:00Z

the expression in step example2 is being passed through as a literal

I was able to fix this by correcting the position of the !expr tag:

    input:
      # Expression with type conversions, math operation, and concatenation
      name: !expr >
        intToString(floatToInt(intToFloat($.input.int_value) *
        $.input.float_value)) + ' User'

Note that the !expr tag goes on the same line before the >. (Take a look at the examples in the old code.)

dbutenhof · 2024-05-21T18:33:27Z

I'm not sure what the three "invalid char" messages are referring to

I got rid of the complaints about the escapes by removing the backslashes from in front of the double quotes in the string....

Leaving the escapes in place and changing the single quotes to double quotes also addresses the complaints....

I'm kinda confused here on exactly what's "ARCA" and what's "YAML"? YAML claims not to support escape sequences inside single quotes, meaning ' \"' would be either uninterpreted or illegal (possibly the problem)? But this is all on an !expr "argument", so does that move all the lexing into ARCA/Go domain?

webbnh · 2024-05-21T18:35:20Z

workflow.yaml:1:16: invalid char escape
workflow.yaml:1:16: invalid char escape
workflow.yaml:1:16: invalid char literal

I got rid of the first two warnings by changing the single quotes in example1 to double quotes and leaving the escapes in place. And, I got rid of the third warning by changing the single quotes in example2 to double quotes, as well.

webbnh

Just attaching my discoveries to the code.

webbnh · 2024-05-21T18:40:24Z

basic-examples/expressions/workflow.yaml

+    input:
+      # Expression with type conversions, math operation, and concatenation
+      name: >
+        !expr intToString(floatToInt(intToFloat($.input.int_value) * 
+        $.input.float_value)) + ' User'


The !expr tag is not part of the data, so it needs to precede the >:

name: !expr > intToString(floatToInt(intToFloat($.input.int_value) * $.input.float_value)) + ' User'

And, the use of single quotes is generating a warning during parsing...but it doesn't seem to adversely affect the operation of the workflow.

webbnh · 2024-05-21T18:42:27Z

basic-examples/expressions/workflow.yaml

-      - name: !expr '"Here''s an apostrophe and \"embedded quotes\"."'
-      - name: !expr "'Here\\'s an apostrophe and \"embedded quotes\".'"
-      - name: !expr |-
-          'Here\'s an apostrophe and "embedded quotes".'
-      - name: !expr |-
-          "Here's an apostrophe and \"embedded quotes\"."
-      - name: !expr '`Here''s an apostrophe and "embedded quotes".`'
-      - name: !expr |-
-          `Here's an apostrophe and "embedded quotes".`
+  example1:
+    plugin:
+      deployment_type: image
+      src: quay.io/arcalot/arcaflow-plugin-template-python:0.4.0
+    input:
+      #Expression with concatenation and string literal
+      name: !expr $.input.name + ' -- \"The Noble Workflow Engine\"'


The escaped double quotes are generating a warning (which doesn't seem to adversely affect the operation of the workflow). There also seems to be a warning (which, again, doesn't seem to have any adverse effect) on the use of the single quotes.

If you replace the single quotes with double quotes, it makes both sets of warnings go away (and, it justifies the escapes...).

changes

c711e3d

mfleader self-assigned this May 3, 2024

jaredoconnell approved these changes May 3, 2024

View reviewed changes

mfleader requested a review from dustinblack May 3, 2024 20:19

This comment was marked as resolved.

Sign in to view

dbutenhof approved these changes May 6, 2024

View reviewed changes

dustinblack previously requested changes May 8, 2024

View reviewed changes

change step name

c7b196a

mfleader added 4 commits May 8, 2024 16:14

add mermaid diagram for parent workflow

dde51e4

Merge branch 'main' into expression-fixes

4ef4db5

add reference to string expressions

86ccf2e

add documentation to workflow

92a7636

mfleader requested a review from dustinblack May 9, 2024 17:45

update workflow with clearer examples and no sub-workflow (#29)

dee4e73

* update workflow with clearer examples and no sub-workflow * addressing PR feedback * updates from PR feedback

Merge branch 'expression-fixes' of github.com:arcalot/arcaflow-workfl…

366e33f

…ows into expression-fixes

mfleader added documentation Improvements or additions to documentation enhancement New feature or request labels May 20, 2024

mfleader requested review from dbutenhof, jaredoconnell and webbnh and removed request for dustinblack May 20, 2024 15:22

jaredoconnell marked this pull request as draft May 20, 2024 15:34

webbnh requested changes May 21, 2024

View reviewed changes

webbnh reviewed May 21, 2024

View reviewed changes

Conversation

mfleader commented May 3, 2024

Changes introduced with this PR

Uh oh!

This comment was marked as resolved.

Uh oh!

dbutenhof left a comment

Choose a reason for hiding this comment

Uh oh!

dustinblack commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfleader commented May 8, 2024

Uh oh!

mfleader commented May 9, 2024

Uh oh!

dustinblack commented May 17, 2024

Uh oh!

jaredoconnell commented May 20, 2024

Uh oh!

webbnh commented May 20, 2024

Uh oh!

webbnh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webbnh commented May 21, 2024

Uh oh!

webbnh commented May 21, 2024

Uh oh!

webbnh commented May 21, 2024

Uh oh!

dbutenhof commented May 21, 2024

Uh oh!

webbnh commented May 21, 2024

Uh oh!

webbnh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dustinblack commented May 8, 2024 •

edited

Loading