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

Document a Breakdown of Configuration #680

Merged

Conversation

MarcusSorealheis
Copy link
Collaborator

@MarcusSorealheis MarcusSorealheis commented Feb 25, 2024

Description

This PR is the beginning of more work to breakdown in a step-by-step fashion how to configure a NativeLink deployment.

Beginning of addressing #681

Type of change

Documentation

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

WIP

Checklist

  • Updated documentation if needed
  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

Copy link

vercel bot commented Feb 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nativelink-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 3, 2024 4:49am

@MarcusSorealheis
Copy link
Collaborator Author

cc @Ely-S and @allada and @blakehatch

more coming.

@MarcusSorealheis MarcusSorealheis changed the title Starts a Breakdown of Configuration Document a Breakdown of Configuration Feb 25, 2024
Copy link
Contributor

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

I like the step wise approach, adding small component visuals might help reader scan document quicker and go back for details, ofc when ready for that

Reviewed all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks

Copy link
Contributor

@blakehatch blakehatch left a comment

Choose a reason for hiding this comment

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

If they're planning on configuring this themselves, it could be helpful to link to a store cheat sheet of sorts, or at least linking to the StoreConfig section of the code: https://github.com/TraceMachina/nativelink/blob/main/nativelink-config/src/stores.rs. I'd probably recommend against pointing to code here but it is all fairly clearly documented in this file and could probably just be ripped out into markdown if needed.

Reviewed all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks

Copy link
Collaborator

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Cargo Dev / ubuntu-22.04, asan / ubuntu-22.04, pre-commit-checks


nativelink-config/examples/example-nl-config.md line 1 at r2 (raw file):

NativeLink uses a JSON file as the configuration format. This section of the

Shouldn't this be named Readme.md?


nativelink-config/examples/example-nl-config.md line 27 at r2 (raw file):

### Store Name

The value of `stores` includes top-level keys, which are the names of stores. The following example, defines the `AC_MAIN_STORE`.

nit: "user supplied names of stores"


nativelink-config/examples/example-nl-config.md line 44 at r2 (raw file):

Once the store has been named and its object exists,
the next key is the type of store. The options are `filesystem`, `memory`, `compression`, `dedup`, `fast_slow`, `verify`, and `experimental_s3_store`.

I'm not sure if we should do this. This will make our documentation out of date really fast if we name them here.

I think what we should do instead is write a new readme for each store. Alternatively we should auto-gen the readme based on the rustdocs.

In fact this entire readme could probably be generated from our rust doc and published to docs.rs.

Copy link
Collaborator Author

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), integration-tests (20.04), pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04


nativelink-config/examples/example-nl-config.md line 1 at r2 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Shouldn't this be named Readme.md?

Done.


nativelink-config/examples/example-nl-config.md line 44 at r2 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

I'm not sure if we should do this. This will make our documentation out of date really fast if we name them here.

I think what we should do instead is write a new readme for each store. Alternatively we should auto-gen the readme based on the rustdocs.

In fact this entire readme could probably be generated from our rust doc and published to docs.rs.

My feeling is that this is almost a tutorial. It will go away or become outdated like our existing docs. The goal is to move the ball forward at all.

@aaronmondal
Copy link
Contributor

FYI the linter errors are visible when you look at the file on github: https://github.com/TraceMachina/nativelink/pull/680/files

@blakehatch
Copy link
Contributor

blakehatch commented Mar 1, 2024

I was able to figure out how to surface this README in the rustdoc so that the example breakdown and the overview of configuration can be easily accessed on the first page of the rust doc. Would then be super easy to share with a link to docs.rs for the config section.

Put up a separate issue for setting up generating this file but I think this is fine for now.

Going to also put up a PR that conveniently surfaces the existing docs for the stores and config using the existing documentation in the code so it doesn't have to be combed through. Would be helpful if this gets landed first so I can point to it in the lib.rs doc settings.

Copy link
Collaborator

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, asan / ubuntu-22.04, pre-commit-checks


nativelink-config/examples/README.md line 123 at r3 (raw file):

      },
      "work_directory": "/tmp/nativelink/work",
      "platform_properties": {

We are not explaining these at all. I feel this is quite important to get right and explain.


nativelink-config/examples/README.md line 158 at r3 (raw file):

  ### Scheduler Name

The value of `schedulers` includes top-level keys, which are the names of stores. The following example, defines the `MAIN_SCHEDULER`.

nit: We should make it more clear that MAIN_SCHEDULER is user defined and has no special meaning other than a unique name.


nativelink-config/examples/README.md line 201 at r3 (raw file):

      "simple": {
        "supported_platform_properties": {
          "cpu_count": "minimum",

ditto. We also need to explain what minimum, exact and priority are and when each should be used.


nativelink-config/examples/README.md line 268 at r3 (raw file):

      "cas": {
        "main": {
          "cas_store": "WORKER_FAST_SLOW_STORE"

We should also explicitly state that these store names must be defined in the stores mapping.


nativelink-config/examples/README.md line 302 at r3 (raw file):

Private Server

This is weird. This is not a requirement of nativelink. It's a best practice. It has the exact same mapping and layout as the public one, but for security reasons these apis should only be available to trusted sources.

Copy link
Collaborator Author

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, asan / ubuntu-22.04, pre-commit-checks


nativelink-config/examples/README.md line 158 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: We should make it more clear that MAIN_SCHEDULER is user defined and has no special meaning other than a unique name.

I agree however, we should have defaults and keys/labels that identify what it is followed by another key for encapsulating configuration values. a common pattern is:

{
  "scheduler_name": "MAIN_SCHEDULER",
  "configuration_data": { <currently the value of the dynamic MAIN_SCHEDULER key>} 
}

There are many many reasons why you want to have consistent key vs a dynamic one, but we are not at the phase to fight that. It's just a placeholder for now.


nativelink-config/examples/README.md line 201 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

ditto. We also need to explain what minimum, exact and priority are and when each should be used.

Agreed. Should I add a TODO. This can be a subsequent PR and can go in the documentation site, which I can do today.


nativelink-config/examples/README.md line 268 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

We should also explicitly state that these store names must be defined in the stores mapping.

This page does not accomplish all of the documentation needs. It is only a step forward again.


nativelink-config/examples/README.md line 302 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

This is weird. This is not a requirement of nativelink. It's a best practice. It has the exact same mapping and layout as the public one, but for security reasons these apis should only be available to trusted sources.

I can change it to specify that it is not a requirement for NativeLink, but again this is an example and its a best practice for security so I think we should keep it for now.

Copy link
Collaborator

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks


nativelink-config/examples/README.md line 201 at r3 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

Agreed. Should I add a TODO. This can be a subsequent PR and can go in the documentation site, which I can do today.

Either is fine.


nativelink-config/examples/README.md line 302 at r3 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

I can change it to specify that it is not a requirement for NativeLink, but again this is an example and its a best practice for security so I think we should keep it for now.

I'd rather just call it out. I don't like the idea of saying "you must use a private server". This especially matters in the event a user only uses the CAS without remote execution, because there'd be no reason to setup a private server.

Copy link
Collaborator

@allada allada left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r6.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), integration-tests (20.04), macos-13, pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, vale

@MarcusSorealheis MarcusSorealheis merged commit 433829c into TraceMachina:main Mar 5, 2024
25 checks passed
MarcusSorealheis added a commit to MarcusSorealheis/nativelink that referenced this pull request Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants