-
Notifications
You must be signed in to change notification settings - Fork 131
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
Document a Breakdown of Configuration #680
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
cc @Ely-S and @allada and @blakehatch more coming. |
There was a problem hiding this 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
There was a problem hiding this 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
c219cbe
to
a2c7114
Compare
a2c7114
to
ac2af84
Compare
ac2af84
to
61df9dc
Compare
61df9dc
to
c815828
Compare
There was a problem hiding this 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.
c815828
to
521056e
Compare
521056e
to
824ae18
Compare
There was a problem hiding this 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.
FYI the linter errors are visible when you look at the file on github: https://github.com/TraceMachina/nativelink/pull/680/files |
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. |
There was a problem hiding this 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.
There was a problem hiding this 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
andpriority
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.
824ae18
to
5720445
Compare
There was a problem hiding this 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.
5720445
to
05ce409
Compare
05ce409
to
cb02d73
Compare
There was a problem hiding this 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 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
cb02d73
to
e432e5d
Compare
fixed the docs.
e432e5d
to
f06b428
Compare
fixes the doc example
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
not work as expected)
How Has This Been Tested?
WIP
Checklist
bazel test //...
passes locallygit amend
see some docsThis change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)