Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Automatic docs for launcher #1044
Automatic docs for launcher #1044
Changes from 3 commits
c2eaa28
1da6e24
4cab978
86736cb
2bddc78
7206fe3
e40b942
94065cd
c6590fd
a9753a5
0c77c75
a44a82b
dac2348
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this 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 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 dodocker run ghcr.io/huggingface/text-generation-inference:latest --help
(which I think has same output)There was a problem hiding this comment.
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)There was a problem hiding this comment.
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 checkmain.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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 withblack
orisort
, they just check but don't change files), and fail if runningtext-generation-launcher --help >> launcher.md
would change the contents oflauncher.md
, hence forcing users to run this command locally.(no strong opinion, I think @Narsil had suggested the approach of pushing post-merge)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 inmain.rs
would trigger a change inlauncher.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)
There was a problem hiding this comment.
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)