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

Clarify process id field ($graph only) #166

Merged
merged 1 commit into from
Jul 28, 2022
Merged

Conversation

kinow
Copy link
Member

@kinow kinow commented May 6, 2022

Closes #117

Looks like the Process section includes/requires another document where the id field is defined. I'm guessing this works similar to OOP in Python/Java, and that I can specify the id in Process.

Will render the site with this change to confirm it's working as expected...

@kinow
Copy link
Member Author

kinow commented May 6, 2022

Website failed to render after this change. Will take a look later 👍 🕐 🛌

@tom-tan
Copy link
Member

tom-tan commented May 6, 2022

I guess it is the matter of document generation rather than the schema itself because id field is already defined in Identified (Process is derived from Identified).

@kinow kinow force-pushed the clarify-process-id branch 2 times, most recently from 23c5fdd to 8460d79 Compare May 7, 2022 05:36
@kinow
Copy link
Member Author

kinow commented May 7, 2022

Hadn't had to play with schema salad yet (looks like I have a lot to learn about it), but here's the command to reproduce the website build error:

schema-salad-tool --debug CommonWorkflowLanguage.yml

The error, basically, is: “schema_salad.avro.schema.SchemaParseException: Field name id already in use”

I couldn't find a way to define the id in Process different than Identified - like you can override a value in OOP in Java/Python. Tried simply defining the id and the error above happened, and saw specialize key, but also not sure if that'd work.

The last commit adds a comment about the id for Process in the Identified record. I think that means all the children nodes will have the same documentation. Not sure if that'd be OK?

image

Bruno

@kinow kinow marked this pull request as ready for review May 7, 2022 05:42
@kinow
Copy link
Member Author

kinow commented May 7, 2022

Also not sure if the text is explaining it well to users. Are there other levels where the id would be useful for end-users?

@tom-tan
Copy link
Member

tom-tan commented May 7, 2022

In my understanding, there are no way to override the definition (including document) of specific fields.
Currently, schema salad provides docParent, docChild and docAfter fields to add extra documents but they are used to add new (sub)section rather than to override existing documents.

@kinow
Copy link
Member Author

kinow commented May 7, 2022

In my understanding, there are no way to override the definition (including document) of specific fields. Currently, schema salad provides docParent, docChild and docAfter fields to add extra documents but they are used to add new (sub)section rather than to override existing documents.

Thank you for clarifying that @tom-tan 🙇

I guess documenting the id differences in the Identified field declaration should be OK then 🙂

@mr-c
Copy link
Member

mr-c commented May 9, 2022

I guess documenting the id differences in the Identified field declaration should be OK then slightly_smiling_face

Identified is inherited in many places, as shown in https://www.commonwl.org/v1.2/cwl.svg

  • Process (and therefore CommandLineTool, ExpressionTool, Workflow, Operation)
  • WorkflowStep
  • WorkflowStepOutput
  • WorkflowStepInput

So it will be tricky to adjust the documentation field in Identified to match all of those contexts.

Your initial try at 2579e8a works with common-workflow-language/schema_salad#535 so I would suggest you wait for that to get approved & released and then it will be easier to re-write doc fields

@kinow
Copy link
Member Author

kinow commented May 9, 2022

Your initial try at 2579e8a works with common-workflow-language/schema_salad#535 so I would suggest you wait for that to get approved & released and then it will be easier to re-write doc fields

Agreed! Thanks @mr-c

@kinow kinow added the blocked label May 10, 2022
Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

@kinow schema-salad https://github.com/common-workflow-language/schema_salad/releases/tag/8.3.20220518163624 has been released, which allows narrowing (and new doc fields) for inherited types. Please make use of that for this PR

@mr-c mr-c removed the blocked label Jun 21, 2022
@kinow kinow force-pushed the clarify-process-id branch 2 times, most recently from d504568 to 8bce022 Compare June 22, 2022 00:39
@kinow
Copy link
Member Author

kinow commented Jun 22, 2022

@kinow schema-salad https://github.com/common-workflow-language/schema_salad/releases/tag/8.3.20220518163624 has been released, which allows narrowing (and new doc fields) for inherited types. Please make use of that for this PR

Tested the latest commit with the following command:

$ pip install schema-salad==8.3.20220525163636 -U
...
$ schema-salad-tool CommonWorkflowLanguage.yml
/home/kinow/Development/python/workspace/cwl-v1.2/venv/bin/schema-salad-tool Current version: 8.3.20220525163636
Schema `CommonWorkflowLanguage.yml` is valid

To confirm it's indeed working as expected, I downgraded schema-salad, and tested it again.

$ pip install schema-salad==8.2.20220204150214
...
Installing collected packages: schema-salad
  Attempting uninstall: schema-salad
    Found existing installation: schema-salad 8.3.20220525163636
    Uninstalling schema-salad-8.3.20220525163636:
      Successfully uninstalled schema-salad-8.3.20220525163636
Successfully installed schema-salad-8.2.20220204150214
$ schema-salad-tool CommonWorkflowLanguage.yml
/home/kinow/Development/python/workspace/cwl-v1.2/venv/bin/schema-salad-tool Current version: 8.2.20220204150214
Schema `CommonWorkflowLanguage.yml` error:
Union item must be a valid Avro schema: {'type': 'record', 'name': 'org.w3id.cwl.cwl.CommandLineTool', 'extends': 'https://w3id.org/cwl/cwl#Process', (...)'doc': 'Exit codes that indicate the process failed due to a permanent logic error, where executing the process with the same runtime environment and same inputs is expected to always fail.\nIf not specified, all exit codes except 0 are considered permanent failure.'}]}

So as @mr-c pointed out, the latest release of schema-salad has enabled the syntax used in the latest commit to have a more specific documentation for the inherited id field 🎉

Thanks!

@mr-c
Copy link
Member

mr-c commented Jun 22, 2022

@kinow Did you make a local rendering and look over the changes?

Guess we should add that to the CI :-)

@kinow
Copy link
Member Author

kinow commented Jun 22, 2022

@kinow Did you make a local rendering and look over the changes?

Guess we should add that to the CI :-)

Not this time. But I think I still have the website checked out (if not, it's a good time to sync it). Will post the rendered output in a bit. That'll be good too to validate what it looks like with the new schema-salad.

@kinow
Copy link
Member Author

kinow commented Jun 23, 2022

I applied the following patch to cwl-website to run the build from this branch with the latest cwltool from quay.io.

diff --git a/site/cwlsite.cwl b/site/cwlsite.cwl
index 08b841f..3a41f1f 100755
--- a/site/cwlsite.cwl
+++ b/site/cwlsite.cwl
@@ -61,7 +61,7 @@ requirements:
 
 hints:
   DockerRequirement:
-    dockerPull: quay.io/commonwl/cwltool_module:main
+    dockerPull: quay.io/commonwl/cwltool_module:3.1.20220607081835
 
 steps:
   make_rdfs:
diff --git a/website.sh b/website.sh
index 628444c..7956423 100755
--- a/website.sh
+++ b/website.sh
@@ -18,9 +18,9 @@ done
 repo=https://github.com/common-workflow-language/cwl-v1.2 \
 bn=$(basename $repo)
 if [[ -d $bn ]] ; then
-    (cd $bn && git fetch origin && git reset --hard origin/main)
+    (cd $bn && git fetch origin && git reset --hard origin/clarify-process-id)
 else
-    git clone $repo && pushd $bn; git checkout main ; git show --no-patch ; popd
+    git clone $repo && pushd $bn; git checkout clarify-process-id ; git show --no-patch ; popd
 fi
 

Then searched for “Only useful for ”. Here's screenshots of where it was rendered in the Command Line Tool page.

image

And here's in the Workflow page.

image

image

image

@mr-c I was going to ask if these looked like the correct places to have this comment… but then I realized that the id field is appearing twice 😅

image

I'm trying to understand what's going on now. Good thing you asked for the rendered output @mr-c 👍

@mr-c
Copy link
Member

mr-c commented Jun 23, 2022

Thanks @kinow ; this will require a fix to schema-salad's makedoc code

@kinow
Copy link
Member Author

kinow commented Jun 25, 2022

Note to self: re-render after schema-salad release is out. We should have a single field then 🤞

@mr-c mr-c force-pushed the clarify-process-id branch from 8bce022 to 7849c09 Compare June 26, 2022 15:37
@kinow
Copy link
Member Author

kinow commented Jun 26, 2022

The website.sh in the cwl-website repository will render the HTML using the schema-salad within the cwltool Docker image, I think. Once a new release of cwltool is made, with schema-salad, it should be possible to re-render the pages in the cwl-website.

But here's a preview from this repository, after updating schema-salad.

pip install schema-salad==8.3.20220626094427
(venv) kinow@ranma:~/Development/python/workspace/cwl-v1.2$ pip list | grep schema-salad
schema-salad       8.3.20220626094427

And running schema-salad-doc with the parameters used in the CWL workflow file from cwl-website:

schema-salad-doc CommonWorkflowLanguage.yml --primtype "#CWLType" --only https://w3id.org/cwl/cwl#WorkflowDoc --only "https://w3id.org/cwl/cwl#Workflow" > /tmp/test.html

image

No duplicate id 😬

@mr-c mr-c force-pushed the clarify-process-id branch from 7849c09 to 74d58fe Compare July 27, 2022 08:08
Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

I can confirm that the rendering works now; thank you @kinow !

@mr-c mr-c merged commit 6f15395 into 1.2.1_proposed Jul 28, 2022
@mr-c mr-c deleted the clarify-process-id branch July 28, 2022 15:15
kinow added a commit to kinow/cwl-v1.2 that referenced this pull request Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clarify the meaning of 'id' at the Process level
3 participants