Data Provenance Revisions #46

gselzer · 2021-11-15T22:18:18Z

Revisions requested by @ctrueden for #35

This commit also adds a BasicHints class, which I would like to use to replace all of the implementations of Hints that we currently have (with the possible exception of ImmutableHints)

It's a static final String, should have been capitalized :)

Looks much nicer that way :)

This method should be used by the implementations, when they know what they're doing. The setHint method can then be exposed to users of the implementations, and the implementations can override this method with their own restrictions (for example, if an implementation wants to forbid a particular type of hint)

We can just assume that simplification, adaptation are allowed when no hint is provided

There isn't much of an argument for it, considering that the OpService can support an OpHistory with minimal effort

This package is designed to house utilities that pertain to implementations of the Ops API classes. It is regrettable that these implementations cannot be left within SciJava Ops Engine (or whatever other module), and we might want to eventually put this package within its own module.

They are really performing two behaviors, so we should name them differently

It is now clearer which generates a new UUID

It is now more robust, changing any character that is not a valid java identifier. There still exists the problem that different types clash, but I think this is unlikely

They don't do anything

By passing it through to the constructor, we only have to determine the version once for all Ops in an OpCollection

This differentiates it from the other op() signature, which does something somewhat different. More work on this front to come...

The most important change here is the change to the javassist portion of OpMethodInfo. The current framework cannot cache the Op returned if called via signature, as we do not have enough to cache a MatchingConditions, but this commit avoids the matching if we have alreaady created the class. This commit should be viewed with scrutiny.

That's a mouthful :) Plus: * Test Adaptation recovery with dependencies * substituteTypeVars: add GenericArrayType support * InfoChainGenerator determinism Signed-off-by: Curtis Rueden <ctrueden@wisc.edu>

We can use the OpHistory for this!

This cleans up OpEnvironment methods

It seemed backwards that one of the public API methods of DefaultOpEnvironment made a RichOp from an OpInstance, just to grab its InfoChain. We can just skip the RichOp creation

ctrueden · 2021-11-20T04:30:23Z

I cleaned up the history so it's based on the latest main branch, squashed the failing commits, and removed WIP labels. Review will need to wait till next week, though.

ctrueden

Whew! That one took a while. There were a lot of intermediate commits with subsequent changes later down the chain. I also reviewed commits out of order because GitHub defaults to date order rather than commit order in the commit view. 😾

I filed an issue for the most major issue: the naming of "op"/OpInfo/OpInstance/InfoChain/RichOp. For the rest, I'll do one more quick scan and file issues as appropriate any followup action items.

scijava/scijava-ops-engine/src/main/java/org/scijava/ops/engine/impl/DefaultOpEnvironment.java

scijava/scijava-ops-api/src/main/java/org/scijava/ops/api/Hints.java

scijava/scijava-ops-engine/src/main/java/org/scijava/ops/engine/hint/AbstractHints.java

scijava/scijava-ops-engine/src/main/java/org/scijava/ops/engine/hint/AdaptationHints.java

scijava/scijava-ops-engine/src/main/java/org/scijava/ops/engine/BaseOpHints.java

scijava/scijava-ops-engine/src/test/java/org/scijava/ops/engine/impl/ProvenanceTest.java

scijava/scijava-ops-engine/src/test/java/org/scijava/ops/engine/hints/AdaptationHintTest.java

scijava/scijava-ops-api/src/main/java/org/scijava/ops/api/features/OpMatchingException.java

scijava/scijava-ops-engine/src/main/java/org/scijava/ops/engine/simplify/SimplifiedOpInfo.java

ctrueden · 2021-12-06T20:53:25Z

OK, everything significant from the review has been filed as issues on the project board. Merge time! 🏎️💨

gselzer · 2022-01-07T22:35:11Z

I was able to resolve a good amount of these comments on the scijava/scijava-ops-engine/data-provenance-revisions-revisions branch! What a mouthful.

I'll continue with the revisions next week.

gselzer mentioned this pull request Nov 15, 2021

Review and merge outstanding scijava-ops-related PRs scijava/scijava#75

Closed

25 tasks

gselzer force-pushed the scijava/scijava-ops-engine/data-provenance-revisions branch from 875e29a to 4965e52 Compare November 16, 2021 19:15

gselzer added 12 commits November 19, 2021 20:51

Rename setHints, add Hint get/create methods

7347be3

This commit also adds a BasicHints class, which I would like to use to replace all of the implementations of Hints that we currently have (with the possible exception of ImmutableHints)

Fix LossReporterWrapper indentation

4b26284

Capitalize prefix

4c444ba

It's a static final String, should have been capitalized :)

Rename Hints.getCopy() -> Hints.copy()

eb52ede

Looks much nicer that way :)

Fix Hints Javadoc typo

3388140

Clean AbstractHints constructors

434f5f9

Restrict random UUID generation to AbstractHints

b59e23a

Remove default Hint options

17d9841

We can just assume that simplification, adaptation are allowed when no hint is provided

Move BaseOpHints to API module

92e8587

Remove OpHistoryService

1984dd6

There isn't much of an argument for it, considering that the OpService can support an OpHistory with minimal effort

Explain hints creation in SimplifiedOpRef

bf703c1

ctrueden force-pushed the scijava/scijava-ops-engine/data-provenance-revisions branch from 4965e52 to 13b6eae Compare November 20, 2021 02:52

gselzer added 15 commits November 19, 2021 22:27

Progress toward unique Op IDs

6243355

Differentiate generateOpInstance methods

f8e1573

They are really performing two behaviors, so we should name them differently

Remove link to Simplification Hints

4bce813

Split Hints.copy() into two methods

ca15664

It is now clearer which generates a new UUID

Prevent instantiation/extension of public utility

4478c10

Improve OpMethodInfo.getParameterName

fff6721

It is now more robust, changing any character that is not a valid java identifier. There still exists the problem that different types clash, but I think this is unlikely

Update module-info.java

f1c17f8

Make ProducerOpWrapper more readable

ec22a87

Remove public modifiers from interfaces

b1b1ec3

They don't do anything

Extract version determination from OpInfos

e60fbf4

By passing it through to the constructor, we only have to determine the version once for all Ops in an OpCollection

Rename op(OpInfo, ...) to opFromInfo

f381204

This differentiates it from the other op() signature, which does something somewhat different. More work on this front to come...

RichOp: first cut

7cdaccf

Rewrite OpExecutionSummary to take RichOps

7ab7ad8

Clean OpHistory class

38d574a

gselzer added 12 commits November 19, 2021 22:28

Improve OpAdaptationInfo constructor

b4bdc97

Work on OpAdaptationInfoChainGenerator

302d57d

That's a mouthful :) Plus: * Test Adaptation recovery with dependencies * substituteTypeVars: add GenericArrayType support * InfoChainGenerator determinism Signed-off-by: Curtis Rueden <ctrueden@wisc.edu>

Progress on SimplificationInfoChainGenerator

ec49244

SimplificationInfoChainGeneration work

2d788ae

More SimplificationInfoChainGenerator work

ab18221

remove InfoChainGenerator.priority

4dc33c1

Remove ugly casts in ProvenanceTest

f9cbd53

We can use the OpHistory for this!

Clean OpEnvironment

c8d195a

Rename opFromID to opFromSignature

354abba

Extract matching Exceptions to SciJava Ops API

a081dfc

This cleans up OpEnvironment methods

Rework findOp(OpInfo, ...) methods

5424aca

It seemed backwards that one of the public API methods of DefaultOpEnvironment made a RichOp from an OpInstance, just to grab its InfoChain. We can just skip the RichOp creation

ctrueden force-pushed the scijava/scijava-ops-engine/data-provenance-revisions branch from 13b6eae to 5424aca Compare November 20, 2021 04:29

ctrueden self-requested a review November 20, 2021 04:30

ctrueden self-assigned this Nov 20, 2021

ctrueden approved these changes Dec 6, 2021

View reviewed changes

ctrueden merged commit 66d08ad into main Dec 6, 2021

ctrueden deleted the scijava/scijava-ops-engine/data-provenance-revisions branch December 6, 2021 20:53

ctrueden removed their assignment Dec 6, 2021

gselzer mentioned this pull request Jan 14, 2022

Revisions on the Revisions of the Data Provenance branch #53

Merged

gselzer mentioned this pull request Oct 24, 2022

OpHints configuration problems/limitations scijava/scijava#101

Closed

gselzer mentioned this pull request Jul 21, 2023

Standardize utility class pattern scijava/scijava#127

Closed

gselzer mentioned this pull request Oct 18, 2023

Make InfoTreeGenerator Prioritized #92

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Data Provenance Revisions #46

Data Provenance Revisions #46

Uh oh!

gselzer commented Nov 15, 2021

Uh oh!

ctrueden commented Nov 20, 2021

Uh oh!

ctrueden left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ctrueden commented Dec 6, 2021

Uh oh!

gselzer commented Jan 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Data Provenance Revisions #46

Data Provenance Revisions #46

Uh oh!

Conversation

gselzer commented Nov 15, 2021

Uh oh!

ctrueden commented Nov 20, 2021

Uh oh!

ctrueden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ctrueden commented Dec 6, 2021

Uh oh!

gselzer commented Jan 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants