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

docs: add documentation for multirm #9016

Merged
merged 6 commits into from
Mar 20, 2024
Merged

Conversation

carolinaecalderon
Copy link
Contributor

@carolinaecalderon carolinaecalderon commented Mar 18, 2024

Reverted

#9053

Description

Update configuration docs & add a release note for multirm.

Test Plan

N/A

Commentary (optional)

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

RM-78

@cla-bot cla-bot bot added the cla-signed label Mar 18, 2024
@determined-ci determined-ci requested a review from a team March 18, 2024 17:51
@determined-ci determined-ci added the documentation Improvements or additions to documentation label Mar 18, 2024
Copy link

netlify bot commented Mar 18, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit cfa5313
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65fb171d7c424100089ab4c4
😎 Deploy Preview https://deploy-preview-9016--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@carolinaecalderon carolinaecalderon changed the title feat: update documentation for multirm docs: add documentation for multirm Mar 19, 2024
@carolinaecalderon carolinaecalderon marked this pull request as ready for review March 19, 2024 12:49
Copy link
Member

@tara-det-ai tara-det-ai left a comment

Choose a reason for hiding this comment

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

added comments w suggested edits

@tara-det-ai tara-det-ai self-requested a review March 19, 2024 13:43
Copy link
Member

@tara-det-ai tara-det-ai left a comment

Choose a reason for hiding this comment

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

see comments

@determined-ci determined-ci requested a review from a team March 20, 2024 13:12
docs/reference/deploy/master-config-reference.rst Outdated Show resolved Hide resolved
docs/reference/deploy/master-config-reference.rst Outdated Show resolved Hide resolved
docs/reference/deploy/master-config-reference.rst Outdated Show resolved Hide resolved
Comment on lines +12 to +20
- WebUI: Add ability to view resource manager name for resource pools.

- API/CLI/WebUI: Route any requests to resource pools not defined in the master configuration to
the default resource manager, not any additional resource manager, if defined.

- Configuration: Add a ``name`` and ``metadata`` field to resource manager section in the master
configuration. Add an ``additional_resource_managers`` section that follows the
``resource_manager`` and ``resource_pool`` configuration pattern.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- WebUI: Add ability to view resource manager name for resource pools.
- API/CLI/WebUI: Route any requests to resource pools not defined in the master configuration to
the default resource manager, not any additional resource manager, if defined.
- Configuration: Add a ``name`` and ``metadata`` field to resource manager section in the master
configuration. Add an ``additional_resource_managers`` section that follows the
``resource_manager`` and ``resource_pool`` configuration pattern.

These notes feel extraneous to me.

I think the release note should be pretty brief and just announce the feature and not get into the details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where else would we put them? I think each of these changes are important for the end user to know -- we make release notes regularly for config changes, and changes in API behavior. We don't currently have a dedicated page to multiRM, so I thought they'd belong here

Copy link
Member

Choose a reason for hiding this comment

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

I also like these updates in release notes for the reasons i posted in earlier comment. release notes are a great place to keep this info. if the users don't like it, they can skip it. it helps internally with cross referencing when we go back in later and check release notes against docs during doc reviews.

docs/release-notes/multirm-for-k8s.rst Outdated Show resolved Hide resolved
docs/release-notes/multirm-for-k8s.rst Outdated Show resolved Hide resolved
Copy link
Member

@tara-det-ai tara-det-ai left a comment

Choose a reason for hiding this comment

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

added suggested edits and resolutions

@determined-ci determined-ci requested a review from a team March 20, 2024 16:58
@carolinaecalderon carolinaecalderon merged commit 39afa3c into main Mar 20, 2024
66 of 78 checks passed
@carolinaecalderon carolinaecalderon deleted the carolinac/multirm-docs branch March 20, 2024 17:23
carolinaecalderon added a commit that referenced this pull request Mar 26, 2024
carolinaecalderon added a commit that referenced this pull request Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants