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

Automate generation of outputs #248

Merged
merged 15 commits into from
Sep 20, 2022
Merged

Automate generation of outputs #248

merged 15 commits into from
Sep 20, 2022

Conversation

kinow
Copy link
Member

@kinow kinow commented Sep 7, 2022

Closes #148

  • Use a directive to run the commands when building the project;
  • Paths are set to whatever structure ReadTheDocs uses, eliminating multiple accounts and home directories appearing in the output (e.g. /home/kinow/ is gone from the examples updated);
  • Update every {code-block} console that uses cwltool or another command that we would like to run during build (have updated a few files from the beginning of the docs, following the Next links… I think up to the second section of Topics).

@kinow
Copy link
Member Author

kinow commented Sep 7, 2022

This PR uses a patched version of https://github.com/sphinx-contrib/sphinxcontrib-runcmd. The patches include:

  • Adds $ to the command executed, similarly to what we have with the {code-block} directive;
  • Adds a parameter to change the working directory before executing the command (i.e. equivalent of cd src/_includes/cwl/ before running the specified command);
  • Redirects stderr to stdout, so we get the exact same output as in a terminal, instead of two concatenated outputs as in the original code;
  • By default uses syntax: bash instead of text (since we will use it for running commands, that makes more sense);
  • Removes ansi colors that are not rendered correctly by Sphinx;

The directive is not actively maintained, with low code activity. I will prepare pull requests over the next weeks/months, but the author may decide not to accept every PR (also, the directive itself is a wrapper around code-block, so we could even build another directive on top of it, instead of patching it, when/if we have more time).

Here's an example of what we had before:

```{code-block} console
:name: running-hello_world.cwl-with-cwltool
:caption: Running `hello_world.cwl` with `cwltool`.

$ cwltool hello_world.cwl
INFO /tmp/venv/bin/cwltool 3.1.20220628170238
INFO Resolved 'hello_world.cwl' to 'file:///tmp/hello_world.cwl'
INFO [job hello_world.cwl] /tmp/yn0e8xu6$ echo \
    'Hello World'
Hello World
INFO [job hello_world.cwl] completed success
{}
INFO Final process status is success
```

Which renders to:

image

With the new directive, we have:

```{runcmd} cwltool hello_world.cwl
:name: running-hello_world.cwl-with-cwltool
:caption: Running `hello_world.cwl` with `cwltool`.
```

Which renders to:

image

@kinow kinow force-pushed the automate-generation-of-outputs branch from 7671261 to 9674a51 Compare September 7, 2022 03:49
@kinow
Copy link
Member Author

kinow commented Sep 7, 2022

As for the second part, I forgot we are building the final version on ReadTheDocs, which already uses Docker 😄

Right now it's not able to locate the included files, so I believe its default working directory is different than what I am using. Will play with that value, but quite close to having a working preview before updating the other files.

@kinow kinow force-pushed the automate-generation-of-outputs branch from 1f85b9b to bc7cf00 Compare September 7, 2022 04:30
@kinow
Copy link
Member Author

kinow commented Sep 7, 2022

Preview of the same part of the site, but now rendered in ReadTheDocs (note that there's no /tmp or /home/kinow):

image

We have full control of the output, so in theory we could even find-replace that value for something else if needed, but that makes the build a little more brittle I guess.

Any thoughts so far @mr-c, @tetron?

@mr-c
Copy link
Member

mr-c commented Sep 7, 2022

This is great to see! A big bonus if the explicit color coding can be maintained instead of random highlighting by the parser.

@kinow
Copy link
Member Author

kinow commented Sep 7, 2022

This is great to see! A big bonus if the explicit color coding can be maintained instead of random highlighting by the parser.

Good idea. I know colorama ansi2html is able to convert the shell output into HTML, let me see if I can put everything together and get the same terminal colors into the browser… 🤞

@kinow
Copy link
Member Author

kinow commented Sep 8, 2022

I quickly hacked the Sphinx directive to import and call ansi2html, before trying to integrate its output into Sphinx rendered output. The ansi2html output doesn't look very promising.

image

Going to push this back to the backburner, for a follow-up 👍 😥

@kinow kinow force-pushed the automate-generation-of-outputs branch 3 times, most recently from a765204 to f377c5d Compare September 8, 2022 02:53
@@ -28,6 +28,7 @@ watch: clean
--ignore='**venv' \
--ignore='**.github' \
--ignore='*.egg-info' \
--ignore='**_includes/**/*.txt' \
Copy link
Member Author

Choose a reason for hiding this comment

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

To avoid the make watch command from refreshing multiple times as the output.txt and other workflow outputs are written to this _includes folder during the build.

```{admonition} Visualization of 1st-workflow.cwl
[![Visualization of 1st-workflow.cwl](https://view.commonwl.org/graph/svg/github.com/common-workflow-language/user_guide/blob/main/_includes/cwl/21-1st-workflow/1st-workflow.cwl)](https://view.commonwl.org/workflows/github.com/common-workflow-language/user_guide/blob/main/_includes/cwl/21-1st-workflow/1st-workflow.cwl)
[![Visualization of 1st-workflow.cwl](https://view.commonwl.org/graph/png/github.com/common-workflow-language/user_guide/blob/a29e7eae0006660946fc705a310b37a21a7e1edc/_includes/cwl/21-1st-workflow/1st-workflow.cwl)](https://view.commonwl.org/graph/png/github.com/common-workflow-language/user_guide/blob/a29e7eae0006660946fc705a310b37a21a7e1edc/_includes/cwl/21-1st-workflow/1st-workflow.cwl)
Copy link
Member Author

Choose a reason for hiding this comment

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

Note the TODO added above, and the change of link ☝️

@kinow
Copy link
Member Author

kinow commented Sep 8, 2022

50% of the pages converted to use {runcmd}. Going to finish it between tomorrow and Monday, then review again the generated pages, and finally confirm the conformance-tests.yml is not missing anything. Then it will be ready for review.

@kinow kinow force-pushed the automate-generation-of-outputs branch from 0d2dc4b to 4cbba09 Compare September 8, 2022 21:44
Also fix a few typos found during review. Replace backticks code blocks by code-block directive. Update conformance tests. Ignore output.txt files.
@kinow kinow force-pushed the automate-generation-of-outputs branch from 4cbba09 to 25b8896 Compare September 8, 2022 22:14
@kinow kinow marked this pull request as ready for review September 8, 2022 22:27
@kinow kinow requested a review from mr-c September 8, 2022 22:27
@kinow
Copy link
Member Author

kinow commented Sep 8, 2022

Ready for review!

Found a few typos here and there, and some examples that were broken. Also found some code blocks that were not using the caption and instead had extra HTML or a paragraph with the name of the file. There were also a few backtick code blocks (i.e. ```) instead of using the directive {code-block}.

Also updated the names of the directories in the src/_includes/cwl folder, replacing the old 05-custom-types by the same name of the markdown file, custom-types. In the markdown file we have :working-directory: src/_includes/cwl/custom-types so that the command doesn't need to include the whole path, but given we have a convention now of directories matching the markdown names, we could even automate that too, by trying to change the working directory if a directory like src/_includes/cwl/${FILE_NAME} exists — for some day 👍

The output of the commands now must be dynamically generated during build by ReadTheDocs 👍

cwl/sphinx/runcmd.py Outdated Show resolved Hide resolved
cwl/sphinx/runcmd.py Outdated Show resolved Hide resolved
src/topics/file-formats.md Outdated Show resolved Hide resolved
src/topics/inputs.md Outdated Show resolved Hide resolved
Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

This is great! Can we finish the cwl-runnercwltool migration?

When that is done, then I'll compare a diff of the rendered output for my final review

@kinow kinow requested a review from mr-c September 13, 2022 20:16
@kinow
Copy link
Member Author

kinow commented Sep 13, 2022

The broken cachecontrol package has been yanked. Additionally I released a new version of schema-salad that will avoid this problem in the future

Thanks for that @mr-c, the build is now fixed on RTD. I've fixed your other feedback. It should be ready for review again :D 🥁 🤞

@mr-c
Copy link
Member

mr-c commented Sep 13, 2022

$ cwltool arguments.cwl arguments-job.yml
INFO /home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/envs/248/bin/cwltool 3.1.20220913185150
INFO Resolved 'arguments.cwl' to 'file:///home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/248/src/_includes/cwl/additional-arguments-and-parameters/arguments.cwl'
ERROR Workflow error, try again with --debug for more information:
Docker is not available for this tool, try --no-container to disable Docker, or install a user space Docker replacement like uDocker with --user-space-docker-cmd.: docker executable is not available

https://common-workflow-languageuser-guide--248.org.readthedocs.build/en/248/topics/additional-arguments-and-parameters.html

@kinow kinow force-pushed the automate-generation-of-outputs branch from 8d2078e to 9b20c53 Compare September 13, 2022 21:05
@kinow kinow force-pushed the automate-generation-of-outputs branch 7 times, most recently from acd68d1 to 99abb19 Compare September 14, 2022 00:22
@kinow kinow force-pushed the automate-generation-of-outputs branch from 99abb19 to 172b8a9 Compare September 14, 2022 00:39
@kinow
Copy link
Member Author

kinow commented Sep 14, 2022

$ cwltool arguments.cwl arguments-job.yml
INFO /home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/envs/248/bin/cwltool 3.1.20220913185150
INFO Resolved 'arguments.cwl' to 'file:///home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/248/src/_includes/cwl/additional-arguments-and-parameters/arguments.cwl'
ERROR Workflow error, try again with --debug for more information:
Docker is not available for this tool, try --no-container to disable Docker, or install a user space Docker replacement like uDocker with --user-space-docker-cmd.: docker executable is not available

https://common-workflow-languageuser-guide--248.org.readthedocs.build/en/248/topics/additional-arguments-and-parameters.html

This one was trickier, especially since we are running RTD builds already inside Docker, and even though docker-in-docker works, I would prefer to avoid that.

The best solution I could think of was udocker, @mr-c. WDYT? I modified the script to re-use the existing CWLTOOL_OPTIONS, adding just the option to remove colors. Then, in RTD settings I set the environment variable to use udocker.

This way, anyone building the User Guide can use udocker, podman, docker, etc. But on RTD, it will always use udocker.

Screenshot from 2022-09-14 12-45-33
Screenshot from 2022-09-14 12-38-53

@mr-c
Copy link
Member

mr-c commented Sep 14, 2022

Does it use Docker properly when rendered with GitHub Pages?

@kinow
Copy link
Member Author

kinow commented Sep 14, 2022

Does it use Docker properly when rendered with GitHub Pages?

Ah, another good catch @mr-c. I've added the environment variable for GH Pages too, since it'll be using a Docker container for building the docs.

I've pushed the commit here, but then added an extra commit and pushed only to my fork. Then enabled the workflow to run on my fork, so that we could have a preview before merging: https://kinow.github.io/user_guide/topics/additional-arguments-and-parameters.html

https://github.com/kinow/user_guide/actions (see commit with message "f" - for some unknown reason all my temporary commits have this "f" message 🤷‍♂️ 😄 ).

Thanks!

@mr-c mr-c force-pushed the automate-generation-of-outputs branch from 4c6c63b to 86106a0 Compare September 20, 2022 13:59
@mr-c
Copy link
Member

mr-c commented Sep 20, 2022

Thank you @kinow !

@mr-c mr-c merged commit 987a3c5 into main Sep 20, 2022
@mr-c mr-c deleted the automate-generation-of-outputs branch September 20, 2022 14:12
@mr-c mr-c mentioned this pull request Sep 20, 2022
2 tasks
@kinow
Copy link
Member Author

kinow commented Sep 20, 2022

Thank you @kinow !

Thank you for reviewing and testing the containers on GH. I liked that GH action improvement to prepull the containers!

@mr-c
Copy link
Member

mr-c commented Sep 21, 2022

I liked that GH action improvement to prepull the containers!

👍 It was necessary to keep the docker pull output out of the cwltool output

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.

Automate generation of output files?
2 participants