[AIR] HuggingFaceTrainer&Predictor implementation
#23876
Conversation
Yard1
requested review from
maxpumperla,
matthewdeng,
richardliaw,
gjoliver,
amogkam and
krfricke
python/ray/ml/train/integrations/huggingface/huggingface_trainer.py
Outdated
Show resolved
Hide resolved
Yard1
removed
the
@author-action-required
python/ray/ml/predictors/integrations/huggingface/huggingface_predictor.py
Outdated
Show resolved
Hide resolved
| model_config = AutoConfig.from_pretrained(model_checkpoint) | ||
| model = AutoModelForCausalLM.from_config(model_config) | ||
| predictor = HuggingFacePredictor( | ||
| pipeline=pipeline( |
There was a problem hiding this comment.
Nice, loving the use of pipeline!
python/ray/ml/train/integrations/huggingface/huggingface_trainer.py
Outdated
Show resolved
Hide resolved
python/ray/ml/examples/huggingface/huggingface_basic_language_modeling_example.py
Outdated
Show resolved
Hide resolved
|
|
||
| def get_train_dataloader(self): | ||
| if self.train_dataset is None: | ||
| raise ValueError("Trainer: training requires a train_dataset.") |
There was a problem hiding this comment.
Is this error message exposed to users? If so, can we have a better message here?
There was a problem hiding this comment.
This is the same message & validation as in HuggingFace.
| if resume_from_checkpoint: | ||
| self._param_dict[ | ||
| "resume_from_checkpoint" | ||
| ] = self._convert_directory_checkpoint_to_sync(resume_from_checkpoint) |
There was a problem hiding this comment.
Ah I see we need this in both as_trainable and setup since we have to support both user-specified resuming and Tune resuming.
If we change the logic here a little bit: https://github.com/ray-project/ray/blob/master/python/ray/ml/trainer.py#L347-L352
to be like this instead
if checkpoint_dir:
config["resume_from_checkpoint"] = Checkpoint.from_directory(checkpoint_dir)
trainer = trainer_cls(**config)then _convert_directory_checkpoint_to_sync only needs to be called in init, right?
There was a problem hiding this comment.
Correct— my suggestion would still resolve both cases right?
There was a problem hiding this comment.
That should work, yeah. Not sure if this change should be made here though, as it would impact all trainers (possibly in unforeseen ways). Let's do that in a followup PR
Co-authored-by: Amog Kamsetty <amogkam@users.noreply.github.com>
python/ray/ml/examples/huggingface/huggingface_basic_language_modeling_example.py
Outdated
Show resolved
Hide resolved
…modeling_example.py
Why are these changes needed?
Implements
HuggingFaceTrainer&HuggingFacePredictor.Related issue number
Checks
scripts/format.shto lint the changes in this PR.