Add how-data-products-are-resolved.md - #72
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a design wiki document describing how product selectors, layers, graph creation, runtime resolution, and ambiguous matches are handled for data products. ChangesData Product Resolution
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design_wiki/seeds/how-data-products-are-resolved.md`:
- Around line 1-3: Update the Markdown formatting in the documented headings and
list blocks, including the ordered list around Lines 37–42, by adding blank
lines before and after each heading and between surrounding prose and every
list. Ensure all affected sections satisfy Markdownlint rules MD022 and MD032
without changing their content.
- Around line 20-26: Rewrite the layer-matching guidance to state that
graph-time resolution generally ignores layer name, then explicitly document the
provider exceptions: provider-produced products match on layer name, and
explicit providers statically use their specified layer path during graph
creation. Ensure the rules clearly distinguish non-provider, provider, and
explicit-provider products.
- Around line 33-36: Clarify the status of the multi-edge selector behavior in
“Contentious Part” by marking it as a proposal, or align the canonical selector
contract in the related design documentation. If adopting the behavior,
explicitly document that a selector may resolve to multiple products and update
the graph-initialization error rule; otherwise retain the
single-match-per-selector rule here.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 56218e8c-39ff-47a3-bec1-ac9316752a16
📒 Files selected for processing (1)
design_wiki/seeds/how-data-products-are-resolved.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Respect markdownlint rules: MD012, MD022, MD031, MD032, MD034, MD040
Files:
design_wiki/seeds/how-data-products-are-resolved.md
**/*.{md,rst,qmd}
📄 CodeRabbit inference engine (AGENTS.md)
Ensure one trailing newline and no trailing whitespace in all documentation files
Files:
design_wiki/seeds/how-data-products-are-resolved.md
🪛 markdownlint-cli2 (0.23.0)
design_wiki/seeds/how-data-products-are-resolved.md
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
[warning] 2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 12-12: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 19-19: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 28-28: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 33-33: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
| - The "layer name" is the name of the lowest ("leaf") layer in the path | ||
| - Each instance of a data product is placed in a data cell | ||
| - Each instance of a data product is therefore transitively associated with a layer path, and layer name | ||
| - These do not have to be constant across instances of a data product |
There was a problem hiding this comment.
I would word this differently. Perhaps: "A product_selector instance may select data products that do not all have the same data layer."
Once assigned to a data product, the data layer is constant.
There was a problem hiding this comment.
That's not quite what I was meaning to say. I meant that one particular data product produced by a particular algorithm might be placed in "spill" when the input to the algorithm was in "spill", then in "mc_event" when the input was in "mc_event". That's independent of what product_selectors might select this data product.
Maybe what I should say is "The layer path and layer name do not have to be the same for all data products in a data product family"?
There was a problem hiding this comment.
Even that isn't right. If the layer can change then the index set also changes, which means we don't have an indexed family.
There was a problem hiding this comment.
Even that isn't right. If the layer can change then the index set also changes, which means we don't have an indexed family.
I believe we actually do have an indexed family. A product_selector will be "evaluated" at graph-initialization time. At that time, a product_selector may match one or more product specifications corresponding to
It is not required that each index set used in the above union must have identically-shaped indices (e.g., one set's indices may look like [Run: 1, Spill: 6] while another set's indices may look like [Run: 1, Subrun: 9, Spill: 8]).
There was a problem hiding this comment.
That is, these do not have to be constant across invocations of the algorithm producing the data product.
| ### Layers Checking / Resolution | ||
| - It is desirable that a CHOF be able to take a given input product from different layers on different iterations | ||
| - On each iteration the output layer of the CHOF is determined by the layers of its inputs | ||
| - Therefore layer name checking must be delayed til runtime, when data flows along the created edges |
There was a problem hiding this comment.
I think the idea of layer-checking needs to be fleshed out a little. For example, at what points in the execution of a single graph iteration does layer-checking need to happen (and why)? Right now I can only think of two scenarios:
- Routing—the initial sending of the data-cell index to the correct providers, which requires inspecting the layer of the data-cell index (currently achieved by the
index_router). - Layer filtering—when a CHOF with at least one input product selector having a specified
layerfield is downstream of a CHOF that does not have a specifiedlayerfield in its input product selectors.
Am I missing other scenarios?
2 participants
how-data-products-are-resolved.mdto the design wiki seeds.product_selectorbehavior.