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

Start automated aggregation and surfacing of internal documentation with Rustdoc #688

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

blakehatch
Copy link
Contributor

@blakehatch blakehatch commented Mar 2, 2024

First PR for leveraging rustdoc to surface existing documentation in the code. This reduces the need for redundant documentation on complex and quickly-evolving systems such as configuration files and stores.

This PR focuses on requested improvements and aggregation of information on configuring Nativelink, as well as establishing rustdoc patterns to use on the rest of the monorepo as it is continuously documented.

The change surfaces the below information from the existing nativelink-config documentation for the rust doc:

  • Example READMEs (For examples from In-flight PR from @MarcusSorealheis & Kubernetes deployment examples)
    • In order to thoroughly document the requested cas.json , scheduler.json, cas.yaml, and scheduler.yaml examples they require updating in one place

After running cargo doc in the repository, this page can be reviewed in the browser at:
file:///Path/to/repo/nativelink/target/doc/nativelink_config/index.html

This change also surfaces existing documentation and mildly enhances with examples for:

  • nativelink-config/stores.rs -> enum StoreConfig

After running cargo doc in the repository, this page can be reviewed in the browser at:
file:///Users/blakehatch/Projects/nativelink/target/doc/nativelink_config/stores/enum.StoreConfig.html

Upcoming PRs will focus on improving the existing now surfaced and aggregated documentation as well as the formatting of the generated Markdown in nativelink-config/src/lib.rs

#681

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

This change is Reviewable

@blakehatch blakehatch added the documentation Improvements or additions to documentation label Mar 2, 2024
@blakehatch blakehatch self-assigned this Mar 2, 2024
Copy link

vercel bot commented Mar 2, 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 2, 2024 1:53am

Copy link
Contributor

@aaronmondal aaronmondal 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 that we're starting to move towards more generated documentation and the added config examples are nice.

The way this is currently set up will break our bazel tests though. Note that we doc targets for the individual crates with bazel build nativelink-config:docs and so on. There are also generated doc tests that run as part of the testsuite, e.g. bazel test nativelink-config:doc_test.

I think it would be good to have the option to generate the docs with either Bazel or Cargo, but if we have to choose one we'll have to use Bazel since we don't have the same reliability for clippy's doc checking in our Cargo build.

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, publish-image, ubuntu-22.04, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04 (waiting on @blakehatch)


nativelink-config/Cargo.toml line 11 at r1 (raw file):

[package.metadata.docs.rs]
readme = "./README.md"

nit: newline


nativelink-config/examples/README.md line 0 at r1 (raw file):
nit: Intentionally empty?


nativelink-config/src/lib.rs line 15 at r1 (raw file):

// limitations under the License.

#![doc = include_str!(concat!(env!("CARGO_MANIFEST_DIR"), "/README.md"))]

Viewing this in the docpage seems a bit overwhelming. Could we move the specific config examples to the respective stores?


nativelink-config/src/stores.rs line 42 at r1 (raw file):

    /// Memory store will store all data in a hashmap in memory.
    ///
    /// **Example JSON Config:**

I believe there needs to be an empty line around the code snippet to make sure that it's formatted correctly.


nativelink-config/src/stores.rs line 63 at r1 (raw file):

    ///
    /// **Example JSON Config:**
    /// ```json

nit: ditto


nativelink-config/src/stores.rs line 89 at r1 (raw file):

    ///
    /// **Example JSON Config:**
    /// ```json

nit: ditto


nativelink-config/src/stores.rs line 169 at r1 (raw file):

    ///
    /// **Example JSON Config:**
    /// ```json

nit: ditto


nativelink-config/src/stores.rs line 198 at r1 (raw file):

    /// store before those stores.
    ///
    /// **Example JSON Config:**

nit:ditto


nativelink-config/src/stores.rs line 245 at r1 (raw file):

    ///
    /// **Example JSON Config:**
    /// ```json

nit: ditto


nativelink-config/src/stores.rs line 320 at r1 (raw file):

    ///
    /// **Example JSON Config:**
    /// ```json

nit: ditto


nativelink-config/src/stores.rs line 358 at r1 (raw file):

    ///
    /// ***Example JSON Config:***
    /// ```json

nit: ditto


nativelink-config/src/stores.rs line 393 at r1 (raw file):

    ///
    /// **Example JSON Config:**
    /// ```json

nit: ditto


nativelink-config/src/stores.rs line 401 at r1 (raw file):

    ///               // 10mb.
    ///               "max_bytes": 10000000,
    ///             

nit: intentional dots?


nativelink-config/src/stores.rs line 425 at r1 (raw file):

    ///
    /// **Example JSON Config:**
    // ```json

nit: ditto


nativelink-config/src/stores.rs line 444 at r1 (raw file):

    ///
    /// **Example JSON Config:**
    /// ```json

nit: ditto


nativelink-config/src/stores.rs line 461 at r1 (raw file):

    ///
    /// **Example JSON Config:**
    /// ```json

nit: ditto


nativelink-config/src/stores.rs line 490 at r1 (raw file):

    ///
    /// **Example JSON Config:**
    /// ```json

nit: ditto


nativelink-config/src/stores.rs line 508 at r1 (raw file):

    ///
    /// **Example JSON Config:**
    /// ```json

nit: ditto

Copy link
Contributor

@aaronmondal aaronmondal 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 4 files at r1.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, publish-image, ubuntu-22.04, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04 (waiting on @blakehatch)

@blakehatch
Copy link
Contributor Author

Yup there goes CI blowing up lol, let me fix I thought rust format would save me 😄

Copy link
Contributor Author

@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.

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), pre-commit-checks, publish-image (waiting on @blakehatch)


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

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: Intentionally empty?

Yes this is the file Marcus is working on for #680

I'm gonna rebase and land this after that's in, this is just there in the interim so there's something to point to.


nativelink-config/src/lib.rs line 15 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Viewing this in the docpage seems a bit overwhelming. Could we move the specific config examples to the respective stores?

Yeah I want to restructure this page, just wanted to get everything in one place to start. Will try to find a good place to put everything and maybe take chunks of each README for the docs.


nativelink-config/src/stores.rs line 42 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I believe there needs to be an empty line around the code snippet to make sure that it's formatted correctly.

There doesn't as we saw yesterday, rustdoc just only supports rust and text code snippets lol.


nativelink-config/src/stores.rs line 401 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: intentional dots?

Fixed, was whitespace. Not sure why it's shown like that by reviewable.

Copy link
Collaborator

@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.

I don't think you should change the config example just yet. I think the more immediate need is for a sections in the docs that enumerates the config options and what all the values mean.

allada
allada previously requested changes Mar 5, 2024
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: Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), pre-commit-checks, publish-image (waiting on @blakehatch)


nativelink-config/src/stores.rs line 65 at r2 (raw file):

    /// ```json
    /// "experimental_s3_store": {
    ///      "region": "eu-north-1",

nit: spacing here vs other examples are inconsistent.


nativelink-config/src/stores.rs line 73 at r2 (raw file):

    ///          "jitter": 0.5,
    ///      },
    ///  "additional_max_concurrent_requests": 10

nit: I think we got rid of this field.


nativelink-config/src/stores.rs line 90 at r2 (raw file):

    /// **Example JSON Config:**
    /// ```json
    /// "verify": {

This example is far to complex, can we do this with just a memory or filesystem store?


nativelink-config/src/stores.rs line 260 at r2 (raw file):

    ///         },
    ///         "slow": {
    ///           "experimental_s3_store": {

nit: Lets not use s3, just stick to memory and/or filesystem in examples.


nativelink-config/src/stores.rs line 371 at r2 (raw file):

    ///     },
    ///     "slow": {
    ///       "experimental_s3_store": {

ditto.


nativelink-config/src/stores.rs line 397 at r2 (raw file):

    /// "shard": {
    ///     "stores": [
    ///         "store": {

This is not right, it's an array with a key-value.


nativelink-config/src/stores.rs line 406 at r2 (raw file):

    ///         },
    ///        },
    ///        "store": {

duplicate.


nativelink-config/src/stores.rs line 473 at r2 (raw file):

    ///     },
    ///     "upper_store": {
    ///       "noop": {}

nit: Add comment here saying this store discards data larger than 128mib.

@MarcusSorealheis
Copy link
Collaborator

Nit: ```json5

@blakehatch blakehatch force-pushed the rustdoc branch 2 times, most recently from 4fed1a0 to d4637e9 Compare March 8, 2024 00:35
@blakehatch
Copy link
Contributor Author

It seems like the lines in lib.rs pulling in the READMEs with relative paths
#![doc = include_str!("../examples/README.md")]
are causing issues with CI and creating the OCI image.

Any idea on a workaround @aaronmondal?

Copy link
Contributor Author

@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.

Yeah putting up a PR for that rn, hopefully they can land relatively close to each other they supplement each other nicely.

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), publish-image, and 9 discussions need to be resolved


nativelink-config/src/stores.rs line 65 at r2 (raw file):

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

nit: spacing here vs other examples are inconsistent.

Done.


nativelink-config/src/stores.rs line 73 at r2 (raw file):

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

nit: I think we got rid of this field.

It's still in all of the examples but looks like the option now is: "multipart_max_concurrent_uploads"


nativelink-config/src/stores.rs line 90 at r2 (raw file):

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

This example is far to complex, can we do this with just a memory or filesystem store?

Yeah I can simplify, was trying to rip examples actually used in our configs when available.


nativelink-config/src/stores.rs line 260 at r2 (raw file):

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

nit: Lets not use s3, just stick to memory and/or filesystem in examples.

Done.


nativelink-config/src/stores.rs line 371 at r2 (raw file):

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

ditto.

Done.


nativelink-config/src/stores.rs line 397 at r2 (raw file):

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

This is not right, it's an array with a key-value.

Done.


nativelink-config/src/stores.rs line 406 at r2 (raw file):

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

duplicate.

Done.


nativelink-config/src/stores.rs line 473 at r2 (raw file):

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

nit: Add comment here saying this store discards data larger than 128mib.

Done.

Copy link
Contributor

@aaronmondal aaronmondal 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 2 of 5 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), publish-image, and 7 discussions need to be resolved (waiting on @blakehatch)


-- commits line 2 at r3:
nit: Shorten title, put detail in message body.


deployment-examples/kubernetes/BUILD.bazel line 8 at r3 (raw file):

        "cas.yaml",
        "scheduler.json",
        "scheduler.yaml",

nit: Is this missing the worker? Maybe a glob pattern would make sense here.

@blakehatch blakehatch dismissed MarcusSorealheis’s stale review April 4, 2024 22:04

Already got in requested changes in docs PR

@blakehatch blakehatch force-pushed the rustdoc branch 2 times, most recently from e6e87e4 to cc77239 Compare April 5, 2024 01:12
@blakehatch blakehatch dismissed allada’s stale review April 5, 2024 01:14

Made nit changes

Copy link
Contributor Author

@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.

Dismissed @allada from a discussion.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 4 discussions need to be resolved


-- commits line 2 at r3:

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: Shorten title, put detail in message body.

Done.


deployment-examples/kubernetes/BUILD.bazel line 8 at r3 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: Is this missing the worker? Maybe a glob pattern would make sense here.

Done.

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.

:lgtm:

Reviewed 1 of 3 files at r3, 3 of 5 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), and 4 discussions need to be resolved (waiting on @blakehatch)

@blakehatch blakehatch force-pushed the rustdoc branch 3 times, most recently from 6b23eb0 to fd8d51c Compare April 8, 2024 20:47
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants