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.

Sign up for GitHub

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

Automatic docs for launcher #1044

Closed
wants to merge 13 commits into from
Closed

Automatic docs for launcher #1044

Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c2eaa28
autodocs
merveenoyan Sep 21, 2023
1da6e24
minor fix and removed credentials
merveenoyan Sep 21, 2023
4cab978
removed GH token env var
merveenoyan Sep 21, 2023
86736cb
added codeblock, include removal of md file
merveenoyan Sep 21, 2023
2bddc78
fixes
merveenoyan Sep 21, 2023
7206fe3
changed actions since it runs on repo level, added cargo build
merveenoyan Sep 21, 2023
e40b942
added if merged
merveenoyan Sep 21, 2023
94065cd
added print and removed launcher.md to test
merveenoyan Sep 21, 2023
c6590fd
added git push back
merveenoyan Sep 21, 2023
a9753a5
removed main.rs change condition to test
merveenoyan Sep 21, 2023
0c77c75
added my fork
merveenoyan Sep 21, 2023
a44a82b
added my fork
merveenoyan Sep 21, 2023
dac2348
added my fork fix
merveenoyan Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions .github/workflows/auto_docs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
name: Automatic Documentation for Launcher

on:
push:
branches:
- 'merveenoyan/autodoc'

jobs:
update_docs:
if: github.event.pull_request.merged == true
runs-on: ubuntu-latest

steps:
- name: Checkout code
uses: actions/checkout@v2

- name: Install Launcher
id: install-launcher
run: |
cd launcher
cargo build
cd ..

- name: Run TGI Launcher docs
id: run-launcher-help
run: |
output=$(text-generation-launcher --help)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the GitHub Actions workflow has anything installed to run text-generation-launcher --help. You will need to either install this in GH Actions or use a Docker GA that allows you to do docker run ghcr.io/huggingface/text-generation-inference:latest --help (which I think has same output)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe include it in the build step too (because there it's available as the docker image)

Copy link
Contributor Author

@merveenoyan merveenoyan Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@osanseviero @Narsil I feel like installing from docker doesn't make sense given text-generation-launcher --help will not check main.rs in that branch, does it? So is the output going to come from the source? I tested it and it was coming from docker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized make install works better for this case.

echo "::set-output name=output::$output"

- name: Update Docs
run: |
output="${{ steps.run-launcher-help.outputs.output }}"
rm /docs/source/basic_tutorials/launcher.md
echo '```' >> /docs/source/basic_tutorials/launcher.md
echo $output >> /docs/source/basic_tutorials/launcher.md
echo '```' >> /docs/source/basic_tutorials/launcher.md
echo "Docs are updated!"

git add launcher.md
git commit -m "Update launcher.md"
git push
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be doing a new push to main, but the workflow triggers with every new push. Wouldn't this trigger an infinite loop? (not sure)

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I think it would be better to make this check that launcher.md is up to date (as we do with black or isort, they just check but don't change files), and fail if running text-generation-launcher --help >> launcher.md would change the contents of launcher.md, hence forcing users to run this command locally.
(no strong opinion, I think @Narsil had suggested the approach of pushing post-merge)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I setup what you said of having a check instead of force-push, and it also works. Both are fine, but indeed it could trigger infinite loop I don't knwo how GH handles those.

Copy link
Contributor Author

@merveenoyan merveenoyan Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't this always be up to date since it runs every time there's a change in main.rs?

rm ../../docs/source/basic_tutorials/launcher.md
echo '```' >> ../../docs/source/basic_tutorials/launcher.md
echo $output >> ../../docs/source/basic_tutorials/launcher.md
echo '```' >> ../../docs/source/basic_tutorials/launcher.md

so whenever there's a change in the file it will always remove the launcher.md and then create new one and post output. Is this what you mean?

Copy link
Contributor Author

@merveenoyan merveenoyan Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I removed git pushes and it will only update after merger to main (I think below is only way to do so? see https://github.com/orgs/community/discussions/26724#discussioncomment-3253093)

on:
  push:
    paths:
      - "../../launcher/src/main.rs"
    push:
      branches:
        - main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will think more on this, I can't push this way either :D

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion was to, rather than GA updating launcher.md directly, just have GA check if the changes in main.rs would trigger a change in launcher.md. E.g. you can do this by checking if output == content in launcher.md. If it's the same, nothing happens. If it's different, then the GA workflow fails.

This means that users, not GA, will be forced to run the command to update the doc (it could be a bash script encapsulating everything as well). (again, no strong opinion, it has its pros and cons)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it like so: (also started working in another PR since in this fork the action is not triggered)

  - name: Update Docs
     run: |
       output="${{ steps.run-launcher-help.outputs.output }}"
       launcher_content=$(/docs/source/basic_tutorials/launcher.md)

       if [ "$launcher_content" != "$output" ]; then
         rm /docs/source/basic_tutorials/launcher.md
         echo '```' >> /docs/source/basic_tutorials/launcher.md
         echo $output >> /docs/source/basic_tutorials/launcher.md
         echo '```' >> /docs/source/basic_tutorials/launcher.md
         echo "Docs are updated!"

         git add launcher.md
         git commit -m "Update launcher.md"
         git push