[CAIKIT-NLP-3] Remove device arg from .run #112
Conversation
rawkintrevo
requested review from
alex-jw-brooks,
gkumbhat and
evaline-ju
as code owners
|
Can someone tell me what this lint thing is doing besides failling and making it impossible to update my code w/o a force push? |
|
Hey @rawkintrevo! This project uses some tools for code formatting / linting (e.g., isort, black, pylon, etc). There's instructions on how to set up & run the code formatter and linter here. It looks like this is failing due to the black code formatter - running the format command and checking in the result should resolve it |
Signed-off-by: Trevor Grant <trevor.d.grant@gmail.com> Fix reference \nTrevor Grant <trevor.d.grant@gmail.com> Fix prompt output type checking Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> Fix prompt output type checking Signed-off-by: Kelly A <kellyaa@users.noreply.github.com> have to fix these for DCO anyway so /shrug have to fix these for DCO anyway so /shrug have to fix these for DCO anyway so /shrug maybe tox finally worked ?! Signed-off-by: root <root@caikit-testing.sl.cloud9.ibm.com>
|
bump @gkumbhat / @alex-jw-brooks could i get a review and this merged? |
| @@ -415,14 +420,26 @@ def train( | |||
| if not tuning_config.output_model_types: | |||
| output_model_types = base_model.PROMPT_OUTPUT_TYPES | |||
| else: | |||
| # If the first element is not PromptOutputModelType, assume the entire list | |||
There was a problem hiding this comment.
Not sure why this change is showing up here, but this was already merged in this PR. This PR might need either rebase or merge from main
| log.warning( | ||
| "Specifying device is deprecated and ignored, please update your calling argument" | ||
| ) | ||
| device = self._DETECT_DEVICE |
There was a problem hiding this comment.
I think instead of this, we should fetch the device information from the model itself, otherwise there is still possibility of discrepancy. Can we instead do self.model.device to fetch the information which device the model is ?
There was a problem hiding this comment.
The old line 177 set device this way, I assumed there was a reason for it?
There was a problem hiding this comment.
Hmm. in grand scheme of things, it was sort of a bug 😄 This parameter should have never been part of run since a user can't really choose device for the data to land on per inference request.
There was a problem hiding this comment.
Yeah, it actually is a bug since the device has to match the model, otherwise it'll throw because the tensors are on different devices. Probably the right behavior here is to remove this parameter from .run (since we are in 0.x still) rather than keep it here, since the only way this parameter can currently be used is by using the model device
Local .run() was kind of added as a convenience, since inference is usually done through the TGIS distributed version of this module, because Peft doesn't support seq2seq models for inference yet
There was a problem hiding this comment.
Thanks @rawkintrevo . Left some suggestions and also it seems this branch needs a rebase since its showing changes from previous PR as net new changes
|
@gkumbhat updated the branch- I think it fell out of sync because it has been waiting for a review for ~2 weeks. Responded to your other comment. |
Disabled calling of device functionality- but left argument in place (defaulted to none) to maintain backwards compatibility. Functionality no longer exists- if user sets device parameter a warning message is shown, otherwise device is detected manually.