docs(helm): Add recommended post-installation configuration guide#556
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: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request adds production-oriented post-install documentation for FiftyOne Enterprise Helm deployments. It introduces a comprehensive guide detailing configuration steps for shared storage prerequisites, dedicated plugins, delegated operator workers, GPU workloads, and plugin registration, along with verification steps. Changes
Estimated Code Review Effort🎯 1 (Trivial) | ⏱️ ~5 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
helm/docs/post-install-recommended-configuration.md (1)
214-215: Normalize orchestrator naming in examples.
teamsDoGpuvsteamsDocpucasing is inconsistent; use a consistent style (e.g.,teamsDoCpu) to reduce copy/paste errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/docs/post-install-recommended-configuration.md` around lines 214 - 215, The example uses inconsistent casing for the orchestrator flag (teamsDoGpu vs teamsDocpu); rename the symbol to a consistent form such as teamsDoCpu across the docs and examples — update the YAML key shown (teamsDocpu -> teamsDoCpu), any other occurrences of teamsDocpu and teamsDoGpu to teamsDoCpu, and ensure related references (replicaCount usage, README examples) match the new name to avoid copy/paste errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/docs/post-install-recommended-configuration.md`:
- Around line 98-100: Update the text string that reads "builtin operators" to
use the hyphenated form "built-in operators" for consistency; locate the
sentence referencing the `teamsDo` pod (the line containing "The `teamsDo` pod
will only be able to execute builtin operators.") and replace "builtin
operators" with "built-in operators".
---
Nitpick comments:
In `@helm/docs/post-install-recommended-configuration.md`:
- Around line 214-215: The example uses inconsistent casing for the orchestrator
flag (teamsDoGpu vs teamsDocpu); rename the symbol to a consistent form such as
teamsDoCpu across the docs and examples — update the YAML key shown (teamsDocpu
-> teamsDoCpu), any other occurrences of teamsDocpu and teamsDoGpu to
teamsDoCpu, and ensure related references (replicaCount usage, README examples)
match the new name to avoid copy/paste errors.
🪄 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: CHILL
Plan: Pro
Run ID: f96a2e8e-9af2-4de0-b9cf-136bdda797ba
⛔ Files ignored due to path filters (1)
helm/values.yamlis excluded by!**/*.yaml
📒 Files selected for processing (4)
helm/README.mdhelm/docs/post-install-recommended-configuration.mdhelm/fiftyone-teams-app/README.mdhelm/fiftyone-teams-app/README.md.gotmpl
| > **Note:** If you are using builtin-only plugin mode, omit the PVC volume | ||
| > mount from the configuration below. The `teamsDo` pod will only be able to | ||
| > execute builtin operators. |
There was a problem hiding this comment.
Use “built-in” for consistency/readability.
Line 100 should use “built-in operators” instead of “builtin operators”.
🧰 Tools
🪛 LanguageTool
[grammar] ~100-~100: Ensure spelling is correct
Context: ...sDo` pod will only be able to > execute builtin operators. To enable delegated operato...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helm/docs/post-install-recommended-configuration.md` around lines 98 - 100,
Update the text string that reads "builtin operators" to use the hyphenated form
"built-in operators" for consistency; locate the sentence referencing the
`teamsDo` pod (the line containing "The `teamsDo` pod will only be able to
execute builtin operators.") and replace "builtin operators" with "built-in
operators".
kevin-dimichel
left a comment
There was a problem hiding this comment.
I thought the intention here was to update the docs so that we suggested a deployment with dedicated plugins and delegated operators? If that's the case, should we move the content from this new helm/docs/post-install-recommended-configuration.md file into the main README.md?
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: John Duncan <30937261+roboav8r@users.noreply.github.com>
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: John Duncan <30937261+roboav8r@users.noreply.github.com>
|
I would like to keep it in a separate
|
kevin-dimichel
left a comment
There was a problem hiding this comment.
We are creating divergence in our approach in docker compose (install with dedicated plugins and DOs [added in https://github.com//pull/583]) and now making that a post-install step for helm.
Is this the right choice?
I feel that we should be prescriptive about the typical default deployment we want to promote (#moreCompute).
By the time someone sees this doc, they've already performed the installation and we are requesting they go back and re-configure. Why not just have them configure it once, the suggested way?
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: John Duncan <30937261+roboav8r@users.noreply.github.com>
roboav8r
left a comment
There was a problem hiding this comment.
Made suggested changes to line length and removed reference to built-in plugins.
kevin-dimichel
left a comment
There was a problem hiding this comment.
I still believe that we should incorporate these recommended configurations into the initial docs and not have a separate doc, but I'll approve anyway.
Rationale
Updating the helm documentation following a recent customer deployment who enabled dedicated plugins and delegated operators (our recommended best practice). This is not currently explicitly stated in the documentation.
Review Priority
Changes
helm/docs/post-install-recommended-configuration.md: astep-by-step guide covering dedicated plugins mode, delegated operators
(always-on and on-demand), optional GPU scheduling, and a plugin quickstart.
Links to the existing deep-dive docs rather than duplicating them.
helm/README.md: new ## Recommended Next Steps section at the end with amotivation paragraph and a three-row summary table (dedicated plugins,
delegated operators, on-demand orchestrator), linking to the new doc.
helm/fiftyone-teams-app/README.md.gotmpl: new ## Recommended Next Stepssection immediately after the helm install command, with the same link.
README.md regenerated via helm-docs.
helm/values.yaml: added a comment above the commented-outdelegatedOperatorDeployments and pluginsSettings blocks pointing to the new
doc.
Checklist
Testing
Documentation-only change. Verified:
helm/docs/ (./plugins-storage.md, ./configuring-delegated-operators.md, ./configuring-gpu-workloads.md, ../../docs/configuring-on-demand-orchestrator.md)helm/README.mdmatches the actual heading structure