Current_incr: abort propagation on constant/equal changes#318
Merged
samoht merged 1 commit intoocurrent:masterfrom Apr 1, 2022
Merged
Current_incr: abort propagation on constant/equal changes#318samoht merged 1 commit intoocurrent:masterfrom
samoht merged 1 commit intoocurrent:masterfrom
Conversation
talex5
approved these changes
Mar 23, 2022
Contributor
talex5
left a comment
There was a problem hiding this comment.
This looks like a strict improvement to me. I never got around to doing this because switching to current_incr was such a huge improvement by itself, but it probably is time to improve things further :-)
Might be worth adding an ?eq argument to Current_incr.map too, but that doesn't need to block this PR.
Thanks!
lib_term/dyn.ml
Outdated
| | Error (_, `Msg m) -> Fmt.pf f "FAILED: %s" m | ||
|
|
||
| let equal_progress x y = match x, y with | ||
| | `Msg x, `Msg y -> x == y |
Contributor
There was a problem hiding this comment.
Might be better as String.equal?
tmcgilchrist
added a commit
to tmcgilchrist/opam-repository
that referenced
this pull request
Apr 7, 2022
…, current_github, current_git, current_examples, current_docker and current (0.6) CHANGES: Core: - Implement labelling of clusters on the Graphviz diagram (@ewanmellor ocurrent/ocurrent#255) - Abort propagation on constant/equal changes (@art-w ocurrent/ocurrent#318) API: - GitHub: Record build status using CheckRun (@tmcgilchrist ocurrent/ocurrent#279) - GitHub: Add details_url to check_run. (@tmcgilchrist ocurrent/ocurrent#282) - GitHub: Add Current_github.Api.cmdliner_opt to allow writing pipelines which can optionally be run as GitHub apps. (@talex5 ocurrent/ocurrent#281) - GitHub: Provide markdown details for CheckRun. (@tmcgilchrist ocurrent/ocurrent#288) - GitHub: Fix wrong name used for repository (@tmcgilchrist ocurrent/ocurrent#289 ocurrent/ocurrent#290) - GitHub: Support Github rebuild via webooks. (@tmcgilchrist ocurrent/ocurrent#283) - GitHub: monitor GraphQL queries (@art-w ocurrent/ocurrent#298) - GitHub: Limit CheckRunStatus summary and text fields to 65535. (@tmcgilchrist ocurrent/ocurrent#300) - GitHub: Log extra context for Webhook validation failure. (@tmcgilchrist ocurrent/ocurrent#302) - GitLab: Initial GitLab plugin work. (@tmcgilchrist ocurrent/ocurrent#299) - Git: Make git reset less verbose (@kit-ty-kate ocurrent/ocurrent#293) Web UI: - Use Lwt.pause instead of Lwt_unix.yield (@MisterDA ocurrent/ocurrent#297) - Use `ansi` instead of `current_ansi` (@samoht ocurrent/ocurrent#321) - Show line numbers and allow jumping to specific lines in job logs (@punchagan ocurrent/ocurrent#309) Docker: - Explicitly set confirmation levels to allow for manually triggered jobs. (@tmcgilchrist ocurrent/ocurrent#304) - Stop using `Dockerfile.t` completely and use strings instead. (@MisterDA ocurrent/ocurrent#301 ocurrent/ocurrent#316) Other: - Update to cohttp 4.0.0 (ocurrent/ocurrent#274, @talex5) - Move `Current_incr` to its own repository (ocurrent/ocurrent#284, @talex5)
Merged
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The mental model of OCurrent is confusing when one comes with the belief that current_incr is driving the show. In practice, code is executed a lot more often than the incremental library would suggest.
This has the merit of forcing users to handle their side effects properly to guard against duplicated execution (with
Current_cacheor custom database upsert logic). As a result, the pipeline works correctly when it's restarted from scratch :)But this does put the pressure on the databases to detect spurious changes! It also makes it impossible to reason locally about the cost of algorithms, as the number of evaluations is dependent on the setup of the global pipeline. The confusion is visible in comments "this is executed twice??" and algorithms that looks "ok" but are actually freezing the pipeline by constantly attempting to update the db with no changes.
The mismatch is that
current_incruses physical equality to detect constant changes, butCurrent_termwraps everything inside aDyn.map, so==is unable to ever be true.This PR is potentially a breaking change as some code will execute less than before!.. But I can't think of existing logic that would break as a result? I've refrained from exposing the
?eqin the signatures, but I would happy to add it if there's a need.