Skip to content

Conversation

@SamuelMarks
Copy link
Collaborator

@SamuelMarks SamuelMarks commented Jul 30, 2025

This PR adds generated API docs.

Checklist

Before submitting this PR, please make sure (put X in square brackets):

  • I have performed a self-review of my code.
  • I have necessary comments in my code, particularly in hard-to-understand areas.
  • I have run end-to-end tests tests and provided workload links above if applicable.
  • I have made or will make corresponding changes to the doc if needed.

Copy link
Collaborator

@shralex shralex left a comment

Choose a reason for hiding this comment

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

Thanks Samuel! Added a few comments. Can we please close this space https://github.com/maxtext and replace references to it from the PR description to the reads-the-docs website.

Copy link
Collaborator

@bvandermoon bvandermoon left a comment

Choose a reason for hiding this comment

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

For my understanding, where do these generated API docs end up? Do we run this manually? Or will it automatically be uploaded to ReadTheDocs?

@SamuelMarks
Copy link
Collaborator Author

Hi - I'm at Quansight Labs and new to the project, and was asked to take a look

I tried building this locally, and can confirm that

First install the dependencies:

$ python3 -m pip install -r requirements_docs.txt

Build

$ sphinx-build -M html docs out

Serve

You can use any static file HTTP server, e.g.:

$ python3 -m http.server -d 'out/html'

works fine for me. The generated API reference in the sidebar looks fine

However, if I click on any API reference item, all I see is something like

image Is this intended, or is the toplevel docstring in that module

""" Static map of TPU names such as v4-8 to properties such as chip layout."""
""" !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
IF YOU MODIFY THIS FILE YOU SHOULD ALSO ADD CORRESPONDING MODIFICATIONS TO
UserFacingNameToSystemCharacteristics in xpk/xpk.py !!!!! """

meant to show up?

Related to that, on the benchmark_chunked_prefill page, I don't see members listed, are we expecting

def benchmark_chunked_prefill(
engine: maxengine.MaxEngine,
params: Any,
chunked_tokens_list: list[chunked_prefill.ChunkedTokens],
):
"""Benchmarks chunked prefill without prefix caching.
Args:
engine: The MaxEngine instance.
params: The model parameters.
chunked_tokens_list: A list of ChunkedTokens objects representing the
input sequence split into chunks.
Returns:
The average time taken for chunked prefill.
"""

to show up?

@MarcoGorelli Thanks for following up. I plan on taking the codebase and going module by module "Gemini: add missing file, module, class, and function docstrings in Google docstring format". Alternatively others can do the same. It should be trivial to maintain 100% doc coverage for this codebase.

As for the specific issues you are coming across, I'll try and replicate. Sometimes sphinx rst's format needs to be explicitly overriden in favour of Google's docstring format; which may be the cause of the issue.

For my understanding, where do these generated API docs end up? Do we run this manually? Or will it automatically be uploaded to ReadTheDocs?

@bvandermoon Thanks for following up. The documentation is auto generated onto readthedocs.

@MarcoGorelli
Copy link

I plan on taking the codebase and going module by module

Sure, if it was expected that with this PR then the listed members wouldn't show up, then no objections

Other than that, there's some commented-out code that's being added here, is that intended?

pyproject.toml Outdated
Comment on lines 257 to 260
#[tool.hatch.metadata.hooks.requirements_txt]
#files = ["requirements.txt"]

Choose a reason for hiding this comment

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

is it intentional to have added this commented-out code?

def setup(app):
run_apidoc(None)
print("running:", app)
# app.connect("builder-inited", run_apidoc)

Choose a reason for hiding this comment

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

should this be un-commented or deleted?

def setup(app):
run_apidoc(None)
print("running:", app)
# app.connect("builder-inited", run_apidoc)

Choose a reason for hiding this comment

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

hi @SamuelMarks

sorry, it's still not clear to me why we're adding this commented-out line

# app.connect("builder-inited", run_apidoc)

is it meant to be uncommented under certain circumstances?

Comment on lines 31 to 32
pip install -e . --no-dependencies
pip install torch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these dependencies needed for building documentation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because torch is imported somewhere in the codebase and sphinx raises a warning about this

Comment on lines +8 to +17
# for import docs
-r base_requirements/requirements.txt
google-tunix>=0.1.1
mlcommons-loadgen
mlperf-logging @ https://github.com/mlcommons/logging/archive/38ab22670527888c8eb7825a4ece176fcc36a95d.zip
tf-keras
torch==2.7.1
trl==0.19.0
vllm
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these needed for docs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because these are each imported somewhere in the codebase and sphinx raises a warning about this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this just be dependencies that are required for the documents themselves? Seems that is how @melissawm set it up previously. If it's just a warning, can we leave it off/turn it off? Or is the recommended approach to actually require the full MaxText set of dependencies?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It gives nice highlighting and documentation hints that would be missed if these were not installed (also the build fails)

…p ; [docs/reference/api.rst] Placeholder for generated API docs ; [pyproject.toml] Uncomment docs deps ; [docs/guides.md] Remove redundant extensions ; [src/MaxText/inference_mlperf/offline_mode.py] Switch from `flags` to `argparse` to overcome duplicate flag error; [.github/workflows/check_docs_build.yml] Use `uv` to resolve max recursive dependency resolution issue ; [src/MaxText/inference_mlperf/matmul/matmul_dtypes.py] Hide under `if __name__ == "__main__"` to resolve hangups on TPUs and CPUs
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.

4 participants