Update run-docs to avoid code duplication #1439
Conversation
Update run-docs to avoid duplicate code
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchchat/1439
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit b77ddf3 with merge base 5684175 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
Add back command explaining seemingly extraneous `echo exit 1`
Update to C++11 ABI for AOTI, similar to ET
mikekgfb
changed the title
Update to run distributed inference test with open-llama instead of llama3.1
Open-llama -> stories to avoid tokens.
.ci/scripts/run-docs
Outdated
| fi | ||
| # Pre-initialize variables | ||
| filepath="" | ||
| parameters="--replace 'llama3:stories15M,-l 3:-l 2' --suppress huggingface-cli,HF_TOKEN" |
There was a problem hiding this comment.
Looks like we have quotation bug making the tests fail silently
python3 torchchat/utils/scripts/updown.py --file torchchat/utils/docs/evaluation.md --replace ''\''llama3:stories15M,-l' 3:-l '2'\''' --suppress huggingface-cli,HF_TOKEN
usage: updown.py [-h] [-f FILENAME] [-p PREDICATE] [-r REPLACE] [-s SUPPRESS]
[-e] [-g]
updown.py: error: unrecognized arguments: 3:-l 2'
There was a problem hiding this comment.
On what system is that? There's definitely something weird going on here. Angela committed a change that we don't need -l 2/3 anymore, so we would not have to switch around that flag? the problem originates because of the space in the arg that should be properly escaped, but some shell does not seem to handle this gracefully.
And yeah, all errors are being suppressed on run-docs commands, so it's been attracting a ton of fails that are surfacing when we look closely
There was a problem hiding this comment.
https://github.com/pytorch/torchchat/actions/runs/12996075947/job/36258706324?pr=1439
https://github.com/pytorch/torchchat/actions/runs/12996075978/job/36258715248?pr=1439
Angela committed a change that we don't need -l 2/3 anymore, so we would not have to switch around that flag?
#1159 Correct for AOTI we should't need the flag
There was a problem hiding this comment.
yeah, all errors are being suppressed on run-docs commands, so it's been attracting a ton of fails that are surfacing when we look closely
Surfacing suppressed errors is a good thing :)
There was a problem hiding this comment.
https://github.com/pytorch/torchchat/actions/runs/12996075947/job/36258706324?pr=1439 https://github.com/pytorch/torchchat/actions/runs/12996075978/job/36258715248?pr=1439
Angela committed a change that we don't need -l 2/3 anymore, so we would not have to switch around that flag?
#1159 Correct for AOTI we should't need the flag
et_run still needs the flag? Can we do something for et_run too? It would both help the users and simplify the cleanup ;)
PS: would -l3 work based on the argline parser? Coz that one doesn't trigger the space in arguments issue that leads to the problem we're seeing. (I think it's a badly implemented shell, but it is what it is, and no point to visit "is there a better shell" and then investigate how to rebuild git around it.... Pragmatically, cheapest way we get over that hump, since iit's sorta a nuisance to say -l anyway.... so if we can just kill it off, that's the best path!)
There was a problem hiding this comment.
So, I've removed -l 3 from aoti_run, and rewritten it as -l3 for et_run. I've also rewritten the aoti_run/et_run commandline parser to allow this behavior (glue flag values onto single letter options) in keeping with traditional POSIX command line processing (where this space is historically optional).
That being saaid, if we don't need -l 2/3 on et_run either, that would be best for our users anyway.
There was a problem hiding this comment.
Thanks for the workaround, agree we should add it for ET (pretty straight forward)
Remove -l 3 since no longer necessary after Angea's change
remove -l 3 from aoti run , and write -l3 for et_run
-l 3:-l 2 -> -l3:-l2 after modifying the command lines. Hopefull this is legal for et_run
Update to support non-space separated args
typo
Add a gs=32 cuda.json for test runs with stories15M
add gs=32 variant of mobile for tests
Use gs=32 variants with stories models
undo gs32
switch to gs=32 quantization (requires consolidated run-docs of pytorch#1439)
Extend timeout to avoid timeout of mps quantization test
enforce that and argument must have at least length 2, and refine check for uniarg (ie arg plus flag value in one option) to be args with more than 2 characters
|
@Jack-Khuu any concerns with landing this after CI/CD runs pass? |
typos
|
Thanks for this, kicked off the CI |
Update run-docs to avoid duplicate code (hygiene for scalability with more doc files)
Also, add support for command line options without space for runner (in line with how POSIX treats commandline arguments) , to offer a more portable way to specify rewrite from readme.md to tests with updown.py.