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

[WIP] Add Github workflow: "Pull Request Docs Check" #2499

Closed
wants to merge 32 commits into from

Conversation

samsrabin
Copy link
Collaborator

@samsrabin samsrabin commented Apr 29, 2024

Description of changes

Adds a Github workflow that checks whether a PR has any documentation build errors.

To resolve existing docs errors in order for this new check to pass:

  • Removes all PTCLM documentation and references.
  • Removes ref to missing figure ndown_ctsm_diagram.svg.

Specific notes

Contributors other than yourself, if any: None

CTSM Issues Fixed (include github issue #): None

Are answers expected to change (and if so in what way)? None

Any User Interface Changes (namelist or namelist defaults changes)? No

Testing performed, if any: In progress on this PR page.

@samsrabin
Copy link
Collaborator Author

samsrabin commented Apr 29, 2024

I think this is going to need to be reworked to include something like

with:
  build-command: "sphinx-build -b html docsrc/ _build -f /tmp/sphinx.log "

but with the command we use. Unfortunately that's not straightforward because of our using the doc_builder library.

@samsrabin
Copy link
Collaborator Author

As of 5008060, the only warnings were this twice:

WARNING: Failed to create a cross reference. Any number is not assigned: figure-mosart-conceptual-diagram

This does NOT happen when building with the Docker image on my Mac. I thus tried to get the workflow to run on the Docker image, and I'm not sure if I succeeded. I definitely see the container getting initialized and torn down, but I can't get that warning to go away, and now it's not marking the check as failed (as I want it to, and as it did in 5008060).

@samsrabin
Copy link
Collaborator Author

@billsacks You're the one who set up the doc-builder system, right? Would you be able to shed any light on this?

@billsacks
Copy link
Member

Sorry, can you clarify what you want me to shed light on? Do you mean #2499 (comment)? If so, that doesn't look familiar and I'm not sure what would be causing it - sorry.

Yes, I created the doc-builder library. Does that seem to be the source of the problems? In this use case, the doc-builder library might be adding more complexity than help: the main things it helps with are (1) Instantiating a docker container for some steps of the build process; and (2) Building versioned documentation in the correct location. (2) seems irrelevant here, and for (1) if you're running this whole GitHub Action within a Docker container (are you?) then you're not gaining much from doc-builder (and if you do use doc-builder, you should do it without the -d argument).

@samsrabin
Copy link
Collaborator Author

Hi @billsacks, looking at my post in the light of day I'm sorry I was so vague! The doc-builder library does NOT seem to be the source of any problem. Rather, I was thinking that indicated you had more experience with building the docs in general, which is what I'm having issues with.

Specifically, the issue is that I want the new Github action to only throw errors/warnings that get thrown when using the Docker method, but it throws a new one: That "Failed to create a cross reference" warning. And it's not just a difference in log level or something, because the Docker method (on my Mac, at least) does actually generate the reference. So I think I'm failing to recreate the environment and/or the build command call, but I'm not sure which.

In trying to troubleshoot, I've made versions of the action that run inside and outside the container. However, both give that warning. I'm actually not even 100% sure the container task is actually running inside the container!

So I guess my question is: If you're familiar enough with Github actions, could you see if there's anything obviously wrong with my setup for the containerized version?

@samsrabin
Copy link
Collaborator Author

Current status: I don't know why the original action (pr-docs-check) runs but the one where I've pointed at my fork (pr-docs-check-ssr, which uses the Docker image escomp/base:latest instead of sphinxdoc/sphinx:2.4.4), doesn't. Specifically, the latter is failing like so:

[sphinx-action] Starting sphinx-action build.
Running: git lfs install && git lfs pull --exclude='' --include=''
====================================
Building docs in doc/source
====================================
Traceback (most recent call last):
  File "/entrypoint.py", line 22, in <module>
    action.build_all_docs(github_env, [os.environ.get("INPUT_DOCS-FOLDER")])
  File "/sphinx_action/action.py", line 152, in build_all_docs
    return_code, annotations = build_docs(github_env.build_command, docs_dir)
  File "/sphinx_action/action.py", line [10](https://github.com/ESCOMP/CTSM/actions/runs/8897422654/job/24432268917?pr=2499#step:4:11)4, in build_docs
    subprocess.check_call(["pip", "install", "-r", docs_requirements])
  File "/usr/lib64/python3.6/subprocess.py", line 306, in check_call
    retcode = call(*popenargs, **kwargs)
  File "/usr/lib64/python3.6/subprocess.py", line 287, in call
    with Popen(*popenargs, **kwargs) as p:
  File "/usr/lib64/python3.6/subprocess.py", line 729, in __init__
    restore_signals, start_new_session)
  File "/usr/lib64/python3.6/subprocess.py", line [13](https://github.com/ESCOMP/CTSM/actions/runs/8897422654/job/24432268917?pr=2499#step:4:14)64, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'pip': 'pip'

Going to try removing all requirements.txt, which shouldn't be needed in the escomp/base Docker container.

@samsrabin
Copy link
Collaborator Author

samsrabin commented Apr 30, 2024

pr-docs-check-ssr no longer complains about missing pip, but it seems unable to create the _build directory. Not sure why! pr-docs-check had no problem doing so (before removing requirements.txt; e.g. in a5d155f). Maybe the escomp/base image doesn't allow writing?

samsrabin added a commit to samsrabin/sphinx-action that referenced this pull request Apr 30, 2024
Trying to avoid problem creating _build here: ESCOMP/CTSM#2499 (comment)
@samsrabin
Copy link
Collaborator Author

Re-running after having the Dockerfile make _build: No difference. Still seeing PermissionError: [Errno 13] Permission denied: '_build'.

@billsacks
Copy link
Member

If you're familiar enough with Github actions, could you see if there's anything obviously wrong with my setup for the containerized version?

I'm not that familiar with GitHub actions, but what you have looks reasonable to me. (You could add something like -j 4 to the build command to speed it up if you have multiple processors, but that's completely unrelated to the issue here.)

Have you checked the sphinx-build command that's printed by the 'make html' step – e.g., something like:

sphinx-build -M html "source" "/Users/sacks/ctsm/docs/versions/master"

I doubt that would shine any light on this, but comparing that command in the GitHub action vs. outside it could confirm that you're invoking things correctly.

@samsrabin
Copy link
Collaborator Author

Thanks, @billsacks! It looks like the issue was actually me not mounting the build directory in the container, but trying to make it IN the container.

@samsrabin
Copy link
Collaborator Author

samsrabin commented May 1, 2024

Well, it works on my machine at least (testing with act: act -W .github/workflows/pr-docs-check.yml). I think I need to change something in my Dockerfile, probably having to do with the source= argument.

@samsrabin samsrabin added next this should get some attention in the next week or two. Normally each Thursday SE meeting. and removed next this should get some attention in the next week or two. Normally each Thursday SE meeting. labels May 1, 2024
@samsrabin samsrabin added simple bfb bit-for-bit and removed simple bfb labels Aug 8, 2024
@samsrabin samsrabin added good first issue simple; good for first-time contributors documentation additions or edits to user-facing documentation test: docs Test documentation build before merging and removed simple good first issue simple; good for first-time contributors labels Oct 3, 2024
@samsrabin
Copy link
Collaborator Author

Closing, as this functionality will come in on #2809.

@samsrabin samsrabin closed this Oct 3, 2024
@samsrabin samsrabin added the closed: duplicate Issue already reported, or changes included in a different PR label Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bfb bit-for-bit closed: duplicate Issue already reported, or changes included in a different PR documentation additions or edits to user-facing documentation test: docs Test documentation build before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants