Use ValueEnricher in ScenarioRunner #10897
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nickchapman-da
force-pushed
the
nick-use-value-enricher-in-scenario-runner
branch
2 times, most recently
from
5456afa to
a75653b
Compare
1st attempt. Causes package recompilation (bad!). CHANGELOG_BEGIN CHANGELOG_END fix build Change ValueEnricher interface to work without passing an Engine ValueEnricher has optional preprocessor simplify new interface to ValueEnricher: caller passes translateValue function
nickchapman-da
force-pushed
the
nick-use-value-enricher-in-scenario-runner
branch
from
a75653b to
41938f8
Compare
Comment on lines
+25
to
+31
| def this(engine: Engine) = { | ||
| this( | ||
| engine.compiledPackages(), | ||
| engine.preprocessor.translateValue, | ||
| engine.loadPackage, | ||
| ) | ||
| } |
There was a problem hiding this comment.
I do not really like this java-like overriding of class constructor.
We are not using it at all in the daml-lf module.
I think this is there in scala for compatibility reason with java, but it is really not very idiomatic.
I would prefer to have an apply method in the companion object.
object ValueEnricher{
def apply(engine: Engine): ValueEnricher =
new ValueEnricher(
engine.compiledPackages(),
engine.preprocessor.translateValue,
engine.loadPackage,
)
}
There was a problem hiding this comment.
Thanks for your comment Remy.
Yeah. I am also not a big fan of these secondary this constructors. The syntax is odd and rather constrained. See below.
So I had already tried your suggestion of the apply method in a companion object. The problem is that all callers must be updated from new ValueEnricher(engine) to ValueEnricher(engine) -- which is fine -- but without the new at the call site, it seems that ValueEnricher looks more like a case class, and obscures the fact that we are dealing with a mutable object.
But my opinions are weakly held here... If we think it is acceptable/better without the new I can make the change.
(
Aside: I found the this syntax is very limited. The following is not allowed syntactically:
def this(engine: Engine) = {
val compiledPackages = engine.compiledPackages()
this(
compiledPackages,
engine.preprocessor.translateValue,
engine.loadPackage,
)
}
-->
daml-lf/engine/src/main/scala/com/digitalasset/daml/lf/engine/ValueEnricher.scala:26: error: 'this' expected but 'val' found.
val compiledPackages = engine.compiledPackages()
^
er, why? 🤷 ... oh of course, scala 😄
)
nickchapman-da
deleted the
nick-use-value-enricher-in-scenario-runner
branch
garyverhaegen-da
added a commit
that referenced
this pull request
Sep 22, 2021
@garyverhaegen-da is in charge of this release. This supersedes the failed #10971 and #10993. Commit log: ``` ced4a27 Include concurrency info in output (#10970) a0b6800 Limit maximum number of concurrent tests (#10969) a0fe0f6 release 1.18.0-snapshot.20210921.7889.1.1b473c2b (#10993) 58c82b6 LF: reduce usage of NodeCreate.coinst (#10988) f56ce2a kvutils: Add structured error data to the error metadata [KVL-1032] (#10964) 3f4dbf6 interfaces: Syntax highlighting for interfaces (#10987) d52a285 interface: Add test for conversion functions (#10991) 28b8d9a bump dotnet (#10979) fe10ffb Use ValueEnricher in ScenarioRunner. (#10897) bdad7d5 Upgrade Java on Windows (#10965) 34390f7 interfaces: Implemented conversions, added test. (#10982) 7740b60 participant-integration-api: Improve the CommandConfiguration docs. (#10978) 04f322e Bump resources for daml-ledger tests (#10984) 6d9922e Retry upsert of command deduplication on oracle and h2 [DPP-609] (#10976) eb4f1b2 LF: Move lookup tests out of EngineTest (#10973) 99836d2 Handle fetchByKey callback correctly in scenario runner (#10980) 23e6a2d Improve feedback from failing match in AuthMatchers (#10981) e299103 [Short] Small test tool's CLI parser refactor (#10923) fc153a9 release 1.18.0-snapshot.20210921.7889.0.1b473c2b (#10971) 1b473c2 Switch trigger service time provider default to wall-clock time (#10967) 4acf34b Add conversions from/to interfaces in Daml parser (#10954) d50df11 interface: Add to/from interface in scala ast (#10960) 9f88e09 Drop jdk8 override (#10963) e0587bc Add a bit of detail to RegisteredTemplates documentation, motivated by: (#10962) cebe6ed Refactor safetyStep in the simplifier. (#10948) ac192fc interfaces: Add to/from interface in Haskell typechecker (#10951) 4b4d7a3 Remove update normalizer which was too aggresive (#10925) 66e1098 Add tests for the `CompletionFromTransaction` converter [KVL-1104] (#10885) 724e50d interfaces: Add to/from interface in Haskell AST (#10945) 9a8d55a Change slack link to discuss wiki. Fixes #10946 (#10947) 8b3b033 LF: Test preprocessor resuming (#10936) 2edfc06 ifaces: name collision, typecheck fetch/exercise (#10896) 5dc15c6 LF: rename language Interface to PackageInterface (#10938) 054c6ab Upgrade canton to a more recent version (#10944) b8533d5 [JSON-API] Production/HA documentation. (#10903) a331762 Clarify `CommandTracker` [KVL-1104] (#10943) 5244643 Changes to increase timeout and remove assertions around mock CommandSubmissionServiceImpl (#10942) 6cc42ee rotate release duty after 1.17.0-snapshot.20210915.7832.0.38227a8e (#10892) 88ef05e sandbox-classic: Only allow `--max-parallel-submissions` here. (#10941) ac02dbd LF: Exhaustive test for valueTranslator. (#10927) 409c0b4 interfaces: Add to/from_interface in proto (#10937) 02c8a9d Split CommonStorageBackend (#10871) dc71a6a update NOTICES file (#10932) 0ba54a4 Add a missing test case to the `CommandTrackerFlowTest` (#10939) 855ecdf [DPP-572] Add ledger API test case for verifying conformance to `--min-tls-version` flag. (#10898) 3e13e3d Switch to stable urls in scoop python manifest (#10933) fc2c87d ledger-api-client: Generate a submission id if it's empty in the `CommandClient` [KVL-1104] (#10926) 04d8f75 Clean up sandbox-on-x conformance tests. (#10766) b4541b5 Logging delay of submitted commands (#10912) b8e21d8 Fix takeFilter for the test StreamConsumer (#10918) 906368d LF: exhaustive test for CommandPreprocessor (#10914) 61d214e Add fetch, exercise implementations for interfaces in speedy. (#10911) d01f8e1 Fix flaky ApiConfigManagementServiceSpec test (#10922) 50291ed interfaces: scala typechecker implementation (#10867) cac8391 Bump ghc-lib to include daml interfaces parser (#10747) b6a6bf7 [Ledger API error codes] Extracted common errors and groups [DPP-607] (#10890) 308f938 Dpp 494 unit testing ha coordinator (#10862) 30f74ad Mark Extractor's VeryLargeArchiveSpec test as flaky (#10916) 9582e01 LF: Refactor PreprocessorSpec test (#10909) 9b0fa29 Separate exercise & fetch for interfaces from templates (#10908) f4adee9 Add conformance test for command deduplication using the CommandService [KVL-1099] (#10883) 8a39118 Rename Completion.deduplication_time to deduplication_duration [KVL-1057] (#10900) 8e22bb6 Drop ContractId typeparameter from Value (#10827) 4543705 Drop cocreature as a codeowner from runtime things (#10906) e6e8147 Release 1.17.0 RC from second to last commit (#10904) f08ac5f Desugar interface implements declarations (#10895) b5648c0 Make `CommandTracker` distinguish submissions of the same command using `submissionId` [KVL-1104] (#10868) ``` Changelog: ``` [Triggers Service] The service now starts by default using wall-clock time instead of static time. If you want to run using static time, you need to do so explicitly using the new '-s' or '--static-time' CLI option. If you were already using '-w' or '--wall-clock-time' the flag has no effect. It's anyway safe to leave it there. Sandbox: Add CLI flag `--min-tls-version` to select minimum enabled TLS version for participant server. ``` CHANGELOG_BEGIN CHANGELOG_END
garyverhaegen-da
added a commit
that referenced
this pull request
Sep 22, 2021
@garyverhaegen-da is in charge of this release. This supersedes the failed #10971 and #10993. Commit log: ``` ced4a27 Include concurrency info in output (#10970) a0b6800 Limit maximum number of concurrent tests (#10969) a0fe0f6 release 1.18.0-snapshot.20210921.7889.1.1b473c2b (#10993) 58c82b6 LF: reduce usage of NodeCreate.coinst (#10988) f56ce2a kvutils: Add structured error data to the error metadata [KVL-1032] (#10964) 3f4dbf6 interfaces: Syntax highlighting for interfaces (#10987) d52a285 interface: Add test for conversion functions (#10991) 28b8d9a bump dotnet (#10979) fe10ffb Use ValueEnricher in ScenarioRunner. (#10897) bdad7d5 Upgrade Java on Windows (#10965) 34390f7 interfaces: Implemented conversions, added test. (#10982) 7740b60 participant-integration-api: Improve the CommandConfiguration docs. (#10978) 04f322e Bump resources for daml-ledger tests (#10984) 6d9922e Retry upsert of command deduplication on oracle and h2 [DPP-609] (#10976) eb4f1b2 LF: Move lookup tests out of EngineTest (#10973) 99836d2 Handle fetchByKey callback correctly in scenario runner (#10980) 23e6a2d Improve feedback from failing match in AuthMatchers (#10981) e299103 [Short] Small test tool's CLI parser refactor (#10923) fc153a9 release 1.18.0-snapshot.20210921.7889.0.1b473c2b (#10971) 1b473c2 Switch trigger service time provider default to wall-clock time (#10967) 4acf34b Add conversions from/to interfaces in Daml parser (#10954) d50df11 interface: Add to/from interface in scala ast (#10960) 9f88e09 Drop jdk8 override (#10963) e0587bc Add a bit of detail to RegisteredTemplates documentation, motivated by: (#10962) cebe6ed Refactor safetyStep in the simplifier. (#10948) ac192fc interfaces: Add to/from interface in Haskell typechecker (#10951) 4b4d7a3 Remove update normalizer which was too aggresive (#10925) 66e1098 Add tests for the `CompletionFromTransaction` converter [KVL-1104] (#10885) 724e50d interfaces: Add to/from interface in Haskell AST (#10945) 9a8d55a Change slack link to discuss wiki. Fixes #10946 (#10947) 8b3b033 LF: Test preprocessor resuming (#10936) 2edfc06 ifaces: name collision, typecheck fetch/exercise (#10896) 5dc15c6 LF: rename language Interface to PackageInterface (#10938) 054c6ab Upgrade canton to a more recent version (#10944) b8533d5 [JSON-API] Production/HA documentation. (#10903) a331762 Clarify `CommandTracker` [KVL-1104] (#10943) 5244643 Changes to increase timeout and remove assertions around mock CommandSubmissionServiceImpl (#10942) 6cc42ee rotate release duty after 1.17.0-snapshot.20210915.7832.0.38227a8e (#10892) 88ef05e sandbox-classic: Only allow `--max-parallel-submissions` here. (#10941) ac02dbd LF: Exhaustive test for valueTranslator. (#10927) 409c0b4 interfaces: Add to/from_interface in proto (#10937) 02c8a9d Split CommonStorageBackend (#10871) dc71a6a update NOTICES file (#10932) 0ba54a4 Add a missing test case to the `CommandTrackerFlowTest` (#10939) 855ecdf [DPP-572] Add ledger API test case for verifying conformance to `--min-tls-version` flag. (#10898) 3e13e3d Switch to stable urls in scoop python manifest (#10933) fc2c87d ledger-api-client: Generate a submission id if it's empty in the `CommandClient` [KVL-1104] (#10926) 04d8f75 Clean up sandbox-on-x conformance tests. (#10766) b4541b5 Logging delay of submitted commands (#10912) b8e21d8 Fix takeFilter for the test StreamConsumer (#10918) 906368d LF: exhaustive test for CommandPreprocessor (#10914) 61d214e Add fetch, exercise implementations for interfaces in speedy. (#10911) d01f8e1 Fix flaky ApiConfigManagementServiceSpec test (#10922) 50291ed interfaces: scala typechecker implementation (#10867) cac8391 Bump ghc-lib to include daml interfaces parser (#10747) b6a6bf7 [Ledger API error codes] Extracted common errors and groups [DPP-607] (#10890) 308f938 Dpp 494 unit testing ha coordinator (#10862) 30f74ad Mark Extractor's VeryLargeArchiveSpec test as flaky (#10916) 9582e01 LF: Refactor PreprocessorSpec test (#10909) 9b0fa29 Separate exercise & fetch for interfaces from templates (#10908) f4adee9 Add conformance test for command deduplication using the CommandService [KVL-1099] (#10883) 8a39118 Rename Completion.deduplication_time to deduplication_duration [KVL-1057] (#10900) 8e22bb6 Drop ContractId typeparameter from Value (#10827) 4543705 Drop cocreature as a codeowner from runtime things (#10906) e6e8147 Release 1.17.0 RC from second to last commit (#10904) f08ac5f Desugar interface implements declarations (#10895) b5648c0 Make `CommandTracker` distinguish submissions of the same command using `submissionId` [KVL-1104] (#10868) ``` Changelog: ``` [Triggers Service] The service now starts by default using wall-clock time instead of static time. If you want to run using static time, you need to do so explicitly using the new '-s' or '--static-time' CLI option. If you were already using '-w' or '--wall-clock-time' the flag has no effect. It's anyway safe to leave it there. Sandbox: Add CLI flag `--min-tls-version` to select minimum enabled TLS version for participant server. ``` CHANGELOG_BEGIN CHANGELOG_END
cocreature
pushed a commit
that referenced
this pull request
Sep 23, 2021
@garyverhaegen-da is in charge of this release. This supersedes the failed #10971 and #10993. Commit log: ``` ced4a27 Include concurrency info in output (#10970) a0b6800 Limit maximum number of concurrent tests (#10969) a0fe0f6 release 1.18.0-snapshot.20210921.7889.1.1b473c2b (#10993) 58c82b6 LF: reduce usage of NodeCreate.coinst (#10988) f56ce2a kvutils: Add structured error data to the error metadata [KVL-1032] (#10964) 3f4dbf6 interfaces: Syntax highlighting for interfaces (#10987) d52a285 interface: Add test for conversion functions (#10991) 28b8d9a bump dotnet (#10979) fe10ffb Use ValueEnricher in ScenarioRunner. (#10897) bdad7d5 Upgrade Java on Windows (#10965) 34390f7 interfaces: Implemented conversions, added test. (#10982) 7740b60 participant-integration-api: Improve the CommandConfiguration docs. (#10978) 04f322e Bump resources for daml-ledger tests (#10984) 6d9922e Retry upsert of command deduplication on oracle and h2 [DPP-609] (#10976) eb4f1b2 LF: Move lookup tests out of EngineTest (#10973) 99836d2 Handle fetchByKey callback correctly in scenario runner (#10980) 23e6a2d Improve feedback from failing match in AuthMatchers (#10981) e299103 [Short] Small test tool's CLI parser refactor (#10923) fc153a9 release 1.18.0-snapshot.20210921.7889.0.1b473c2b (#10971) 1b473c2 Switch trigger service time provider default to wall-clock time (#10967) 4acf34b Add conversions from/to interfaces in Daml parser (#10954) d50df11 interface: Add to/from interface in scala ast (#10960) 9f88e09 Drop jdk8 override (#10963) e0587bc Add a bit of detail to RegisteredTemplates documentation, motivated by: (#10962) cebe6ed Refactor safetyStep in the simplifier. (#10948) ac192fc interfaces: Add to/from interface in Haskell typechecker (#10951) 4b4d7a3 Remove update normalizer which was too aggresive (#10925) 66e1098 Add tests for the `CompletionFromTransaction` converter [KVL-1104] (#10885) 724e50d interfaces: Add to/from interface in Haskell AST (#10945) 9a8d55a Change slack link to discuss wiki. Fixes #10946 (#10947) 8b3b033 LF: Test preprocessor resuming (#10936) 2edfc06 ifaces: name collision, typecheck fetch/exercise (#10896) 5dc15c6 LF: rename language Interface to PackageInterface (#10938) 054c6ab Upgrade canton to a more recent version (#10944) b8533d5 [JSON-API] Production/HA documentation. (#10903) a331762 Clarify `CommandTracker` [KVL-1104] (#10943) 5244643 Changes to increase timeout and remove assertions around mock CommandSubmissionServiceImpl (#10942) 6cc42ee rotate release duty after 1.17.0-snapshot.20210915.7832.0.38227a8e (#10892) 88ef05e sandbox-classic: Only allow `--max-parallel-submissions` here. (#10941) ac02dbd LF: Exhaustive test for valueTranslator. (#10927) 409c0b4 interfaces: Add to/from_interface in proto (#10937) 02c8a9d Split CommonStorageBackend (#10871) dc71a6a update NOTICES file (#10932) 0ba54a4 Add a missing test case to the `CommandTrackerFlowTest` (#10939) 855ecdf [DPP-572] Add ledger API test case for verifying conformance to `--min-tls-version` flag. (#10898) 3e13e3d Switch to stable urls in scoop python manifest (#10933) fc2c87d ledger-api-client: Generate a submission id if it's empty in the `CommandClient` [KVL-1104] (#10926) 04d8f75 Clean up sandbox-on-x conformance tests. (#10766) b4541b5 Logging delay of submitted commands (#10912) b8e21d8 Fix takeFilter for the test StreamConsumer (#10918) 906368d LF: exhaustive test for CommandPreprocessor (#10914) 61d214e Add fetch, exercise implementations for interfaces in speedy. (#10911) d01f8e1 Fix flaky ApiConfigManagementServiceSpec test (#10922) 50291ed interfaces: scala typechecker implementation (#10867) cac8391 Bump ghc-lib to include daml interfaces parser (#10747) b6a6bf7 [Ledger API error codes] Extracted common errors and groups [DPP-607] (#10890) 308f938 Dpp 494 unit testing ha coordinator (#10862) 30f74ad Mark Extractor's VeryLargeArchiveSpec test as flaky (#10916) 9582e01 LF: Refactor PreprocessorSpec test (#10909) 9b0fa29 Separate exercise & fetch for interfaces from templates (#10908) f4adee9 Add conformance test for command deduplication using the CommandService [KVL-1099] (#10883) 8a39118 Rename Completion.deduplication_time to deduplication_duration [KVL-1057] (#10900) 8e22bb6 Drop ContractId typeparameter from Value (#10827) 4543705 Drop cocreature as a codeowner from runtime things (#10906) e6e8147 Release 1.17.0 RC from second to last commit (#10904) f08ac5f Desugar interface implements declarations (#10895) b5648c0 Make `CommandTracker` distinguish submissions of the same command using `submissionId` [KVL-1104] (#10868) ``` Changelog: ``` [Triggers Service] The service now starts by default using wall-clock time instead of static time. If you want to run using static time, you need to do so explicitly using the new '-s' or '--static-time' CLI option. If you were already using '-w' or '--wall-clock-time' the flag has no effect. It's anyway safe to leave it there. Sandbox: Add CLI flag `--min-tls-version` to select minimum enabled TLS version for participant server. ``` CHANGELOG_BEGIN CHANGELOG_END
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use
ValueEnricherinScenarioRunner.ValueEnricherworks without anEngine(whichScenarioRunnerdoes not have).ScenarioRunnerneeds direct access toValueTranslatorrather than via thePreprocessorwrapper, which only works withMutableCompiledPackages(which again,ScenarioRunnerdoes not have).This change advances the state of #6665