Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove digest artifacts from sequence #769

Merged
merged 2 commits into from
Jul 26, 2023
Merged

Conversation

swalker2m
Copy link
Contributor

The ODB API will be updated to provide execution / digest and execution / config queries. The former (digest) returns summary information for which it is required to examine the entire sequence (a fold over all the atoms). The later (config) returns the familiar nextAtom, possibleFuture, and hasMore for each of the acquisition and science sequences. This enables the digest information to be neatly cached while the expanded config can be quickly computed upon request thanks to the tight futureLimit that is imposed upon the sequences.

This PR enables the pending ODB update by moving the atomCount (and setupTime) to the digest.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.13% 🎉

Comparison is base (c802b8a) 78.61% compared to head (6b483c3) 78.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #769      +/-   ##
==========================================
+ Coverage   78.61%   78.74%   +0.13%     
==========================================
  Files         112      112              
  Lines        1487     1487              
  Branches        4        3       -1     
==========================================
+ Hits         1169     1171       +2     
+ Misses        318      316       -2     
Files Changed Coverage Δ
...a/core/model/sequence/arb/ArbExecutionConfig.scala 87.50% <ø> (-1.39%) ⬇️
...core/model/sequence/arb/ArbExecutionSequence.scala 75.00% <100.00%> (-6.82%) ⬇️
...ma/core/model/sequence/arb/ArbSequenceDigest.scala 75.00% <100.00%> (+3.57%) ⬆️

... and 6 files with indirect coverage changes

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

@rpiaggio
Copy link
Contributor

So the digest is now parallel to execution instead of embedded?

Copy link
Contributor

@rpiaggio rpiaggio left a comment

Choose a reason for hiding this comment

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

LGTM

@swalker2m
Copy link
Contributor Author

So the digest is now parallel to execution instead of embedded?

You can ask for both in the same query (and both are now embedded in execution as siblings) but it facilitates the implementation to separate that which needs to read the entire sequence from that which needs only the first few steps.

query {
  observation(observationId: "o-100") {
    execution {
      digest { ... }
      config { ... }
    }
  }
}

@swalker2m swalker2m merged commit 9b8db9a into master Jul 26, 2023
9 checks passed
@swalker2m swalker2m deleted the remove-digest-artifacts branch July 26, 2023 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants