-
Notifications
You must be signed in to change notification settings - Fork 75
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
Replace readme paths with urls #1097
Conversation
Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review. Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed. |
Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
try: | ||
model.apply(disable_quantization) | ||
yield | ||
finally: |
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.
model.apply(enable_quantization)
will always runs regardless of try being success or fail. Is this intended.
Also if it fails I would add a log
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.
model.apply(enable_quantization) will always runs regardless of try being success or fail. Is this intended.
Yes, that is exactly the intention of this change
Also if it fails I would add a log
If an exception is raised, the user will see the raised exception
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.
sorry why do we want to run model.apply(enable_quantization)
if it raises exception? Wouldn't it just exit?
And as long as the exception contains info that we can get from this function, then it should be ok. Ex. have input args useful to log causing the error.
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.
sorry why do we want to run model.apply(enable_quantization) if it raises exception? Wouldn't it just exit?
For example, if the sequential pipeline fails, an exception will be thrown and then caught, in order to fall back to the layer-wise sequential pipeline. This is example of where an exception is raised but caught, and I do not want do have to think about the weird interactions that would result from this context not being properly exited.
This "try finally" pattern is, in general, good for contexts. It allows the user to not have to worry about if the context was exited properly, reducing mental load.
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.
Could we move this code change to a separate PR, ideally I would like the scope of this PR to be limited to updating readme paths
Purpose
.md
extension are not listed in the MANIFEST.in, meaning that they will not be included in the LLM Compressor pypi package. This means that references to these files are left dangling for users who have installed from the pypi package. Rather than including.md
in the package and having to also ship all the large images files associated with them, this PR moves the references to urls hosted by github.md
files, as these files will not be included in the pypi distributionsrc/llmcompressor/transformers/finetune/README.md
src/llmcompressor/pipelines/sequential/README.md
Changes
DisableQuantization
to better catch cases where exceptions such as tracing exceptions are triggeredTesting