Skip to content

refactor: use cwltool loader for standardized cwl doc handling - #130

Open
amaltaro wants to merge 4 commits into
DIRACGrid:mainfrom
amaltaro:fix-108
Open

refactor: use cwltool loader for standardized cwl doc handling#130
amaltaro wants to merge 4 commits into
DIRACGrid:mainfrom
amaltaro:fix-108

Conversation

@amaltaro

@amaltaro amaltaro commented Jul 14, 2026

Copy link
Copy Markdown

Fixes #108

With this pull request, we adopt the cwltool loader functionality to normalize and handle CWL documents, instead of manually juggling with fields and different data types.

Tested with pixi run dirac-cwl-run test/workflows/crypto/description.cwl --print-workflow, where exactly the same workflow visualization is retained.
Also added some unit tests that can be triggered with pixi run pytest test/test_job_executor.py

@amaltaro
amaltaro requested review from aldbr and ryuwd July 14, 2026 09:48

@aldbr aldbr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for your contribution!
I just have some minor comments 🙂

Comment thread src/dirac_cwl/job/executor/__main__.py Outdated
# Show basic info
cwl_version = cwl.get("cwlVersion", "Unknown")
doc = cwl.get("doc", cwl.get("label", ""))
cwl_version = getattr(cwl, "cwlVersion", "Unknown")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just wondering if getattr is not misleading in this context, as I think the cwl instance you get will always have cwlVersion, doc, inputs, .... To double check, but I think they just exist with default value like None or [] if nothing is provided.

This would likely work:

Suggested change
cwl_version = getattr(cwl, "cwlVersion", "Unknown")
cwl_version = cwl.cwlVersion # default value is likely 1.2, to double check
cwl_version = cwl.cwlVersion or "Unknown" # if there is no default value

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for the review, @aldbr .
Yes, directly accessing the object attributes should work as well. However, the getattr adds some protection in case the attribute changes for any reason (bug, non-backward compatible changes, etc).

What is the preferred behavior for this module:
a) have a clear exception being raised in case one of the expected attributes is not to be found?
b) or to gracefully deal with this with getattr and default values?

Given that this is already in a try/except block, I think option a) (which is your suggestion) is better. But better to get a confirmation please.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We discussed and we think the first option is the way to go

Comment thread src/dirac_cwl/job/executor/__main__.py Outdated

console.print()

# Show final outputs (handle both dict and list formats)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess this comment is not true anymore?

Suggested change
# Show final outputs (handle both dict and list formats)

@amaltaro

Copy link
Copy Markdown
Author

@ryuwd @aldbr I applied the changes requested during review.

However, one of the CI tests fails with an error unrelated to this PR:
https://github.com/DIRACGrid/dirac-cwl/actions/runs/30002435046/job/89190520820?pr=130

It looks like another PR that has recently been merged is not failing these unit tests (commit message length). Please let me know how to proceed here and/or how to retrigger CI tests.

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.

[CWL Executor] use cwl_utils to print the workflow

3 participants