Proposal: Improved ProvenanceProfile definition

[fmigneault]

@mr-c

I would like to propose the following changes to the PROV utilities.

The reasoning behind the requested changes is that I am in the process of implementing PROV in crim-ca/weaver#778 such that OGC API - Processes can be extended to demonstrate PROV capabilities (i.e.: https://docs.ogc.org/DRAFTS/24-051.html#_requirements_class_provenance), which can build upon the great work from the CWL community. This would allow the Geospatial community to improve traceability and understanding of their processing pipelines.

However, while I'm able to enable the PROV features and get the resulting metadata, I end up in a situation where the resulting tool and execution PROV files generated do not reflect the reality of what happened with the CWL workflow run, since all the details about the remote server where they are running, the actual users employed by worker instances crunching the data, weaver dispatching the workflow sequencing/resolution to cwltool, or any intermediate transformations from "geo data sources" to CWL-compatible inputs is not reported anywhere.

In the current code state, the ProvenanceProfile class is the one that modifies the PROV document (and with which I would need to extended entities/agents/relationships). This class generates the resulting metadata all within the job execution, and is not easily accessible from "outside" cwltool steps. The only interface that I can access part of the references is are the LoadingContext.research_obj and RuntimeContext.research_obj (along some other arguments like orcid).

Therefore, this PR delegates the creation of the ProvenanceProfile instance to LoadingContext, such that I can create a derived LoadingContext that extends the profile with definitions that are more aligned with weaver and cwltool working together. From the point of view of cwltool, the operations resolve exactly the same way as before.

Let me know if you have any question or if anything should be adjusted.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.73%. Comparing base (f1d192d) to head (d9f7060).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2082      +/-   ##
==========================================
+ Coverage   84.23%   84.73%   +0.49%     
==========================================
  Files          46       46              
  Lines        8334     8337       +3     
  Branches     1961     1960       -1     
==========================================
+ Hits         7020     7064      +44     
+ Misses        838      805      -33     
+ Partials      476      468       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mr-c]

@fmigneault I am very happy to see that you want to implement CWLProv on your engine, and help refactoring this code is very welcome!

[mr-c]

@fmigneault Can you add some more unit tests to increase the test code coverage?

[fmigneault]

@mr-c
I've added some tests to validate that provided provenance user is employed.
I'm not sure how to test specifically the override of the new functions without involving complicated mocks, but at least adding the provenance options should ensure the calls go through the conditions leading to their insertion in the provenance files.

[fmigneault]

Digging deeper into how the user provenance details were set with/without the --orcid option, I actually encountered a situation that I believe is a bug. Running a CommandLineTool directly without the --orcid flag but with --enable-user-provenance/--enable-host-provenance caused the user/host details to actually be omitted, since host_provenance=False, user_provenance=False were explicitly set in the SingleJobExecutor (to avoid duplicating them as workflow step), but the operation to set them in the first place when it is not a Workflow never occurred.

I took the opportunity to refactor slightly the user-provenance strategy, since the code was partially duplicated between the ProvenanceProfile and the ResearchObject classes.

[mr-c]

I love it when additional testing leads to fixes! Thank you @fmigneault .

Shall I merge as 1 big commit, or do you want to rebase this as several clean commits?

[fmigneault]

A rebase is better IMO. I prefer to keep the edit history of the attempts performed.
However, GitHub seems to say there are conflicts making the rebase not possible.
Any idea about what to do?

[mr-c]

A rebase is better IMO. I prefer to keep the edit history of the attempts performed. However, GitHub seems to say there are conflicts making the rebase not possible. Any idea about what to do?

Oh, by rebase I meant that you would locally create a series of cleaner commits using git fetch --all ; git rebase -i origin/main && git push -f or similar.

The edit history will be retained in this PR, but we don't need separate commits about "fix linting" in the main history 😊

Otherwise I can squash and merge as a single commit, which will still preserve the original commit history in this PR.

[fmigneault]

The squash is good then :) you can proceed whenever desired

[fmigneault]

Thanks :)

[fmigneault]

Please let me know once a tag is available so I can pin it on my end.

[mr-c]

You are welcome @fmigneault ; the release is up at https://pypi.org/project/cwltool/3.1.20241217163858/ a.k.a https://github.com/common-workflow-language/cwltool/releases/tag/3.1.20241217163858