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.
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.
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.
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.
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.
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
.mdextension 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.mdin 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.mdfiles, as these files will not be included in the pypi distributionsrc/llmcompressor/transformers/finetune/README.mdsrc/llmcompressor/pipelines/sequential/README.mdChanges
DisableQuantizationto better catch cases where exceptions such as tracing exceptions are triggeredTesting