Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[AIR] <Part 3> Add LightningPredictor to support batch prediction #33196

Merged
merged 21 commits into from
Mar 23, 2023
Merged

[AIR] <Part 3> Add LightningPredictor to support batch prediction #33196

merged 21 commits into from
Mar 23, 2023

Conversation

woshiyyya
Copy link
Member

@woshiyyya woshiyyya commented Mar 10, 2023 •
edited
Loading

Why are these changes needed?

There will be a list of PRs for PyTorch Lightning Integration. This is the second one.

  • LightningTrainer + Test
  • Logging and Checkpointing + Test
  • Predictor + Batch Prediction + Test
  • Release Tests + Docstring tests

The LightningPredictor is a subclass of TorchPredictor. They shares the same interfaces for predictor and batchPrediction.

Content for this PR:

  • Implemented LightningPredictor, loading from LightningCheckpoint
  • Tested simple prediction with different fs and different input format
  • Tested BatchPrediction compatibility

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

woshiyyya added 8 commits March 8, 2023 17:48
@woshiyyya
init Lightning Trainer with ci tests
4c8edc1 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya
add test for trainer with categorical ray dataset
e0fd5d9 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya
fix lightiningEnvironment import error
ebea347 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya
fix test_lightning_trainer not found error
01f9461 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya
format code
22725b6 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya
init LightningCheckpoint
6ba0577 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya
fix linting issues
dde861d 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya
fix lint again
55b7916 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya woshiyyya marked this pull request as ready for review March 10, 2023 03:44
@woshiyyya
init LightningPredictor + tests
a6f9801 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya woshiyyya force-pushed the air/lightning_predictor branch from 1dd72a5 to a6f9801 Compare March 10, 2023 03:52
@woshiyyya
Change lightning_config to dict type to aligh with Ray Tune
e1b9d81 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya
fix non_monitored_checkpoint saving error
2e6500f 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya
Merge branch 'air/lightning_log_and_checkpoint' into air/lightning_pr…
bde880a 
…edictor
@woshiyyya
minor change
9f827fa 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya
Merge remote-tracking branch 'upstream/master' into air/lightning_pre…
d5b2a71 
…dictor
@woshiyyya
update Predictor
cf3b325 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya woshiyyya force-pushed the air/lightning_predictor branch from 207f43d to cf3b325 Compare March 21, 2023 19:46
justinvyu
justinvyu requested changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@justinvyu justinvyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, some suggestions:

python/ray/train/tests/test_lightning_predictor.py Outdated Show resolved Hide resolved
python/ray/train/tests/test_lightning_predictor.py Outdated Show resolved Hide resolved
python/ray/train/tests/test_lightning_predictor.py Outdated Show resolved Hide resolved
python/ray/train/tests/test_lightning_predictor.py Outdated Show resolved Hide resolved
python/ray/train/tests/test_lightning_predictor.py Show resolved Hide resolved
python/ray/train/lightning/lightning_predictor.py Show resolved Hide resolved
python/ray/train/lightning/lightning_predictor.py Outdated Show resolved Hide resolved
python/ray/train/lightning/lightning_predictor.py Outdated Show resolved Hide resolved
@woshiyyya
address review comments
37940f0 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya
import fixture
d48b7bf 
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya
Merge remote-tracking branch 'upstream/master' into air/lightning_pre…
a10d989 
…dictor
gjoliver
gjoliver approved these changes Mar 22, 2023
View reviewed changes
Copy link
Member

@gjoliver gjoliver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks cool. just 1 minor nit.

python/ray/train/lightning/lightning_predictor.py Outdated
**load_from_checkpoint_kwargs: Arguments to pass into
``pl.LightningModule.load_from_checkpoint``
"""
model = checkpoint.get_model(model, **load_from_checkpoint_kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you don't need to unpack then pack. just pass it like:

model = checkpoint.get_model(
    model, load_from_checkpoint_kwargs=load_from_checkpoint_kwargs
)

will do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Updated!

@woshiyyya
fix lint
9201677 
Signed-off-by: Yunxuan Xiao <xiaoyunxuan1998@gmail.com>
@justinvyu justinvyu self-requested a review March 23, 2023 03:31
justinvyu
justinvyu approved these changes Mar 23, 2023
View reviewed changes
Copy link
Contributor

@justinvyu justinvyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅

@gjoliver gjoliver merged commit 806ff6b into ray-project:master Mar 23, 2023
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
@woshiyyya @elliottower
[AIR] <Part 3> Add LightningPredictor to support batch prediction (ra…
1fe37bd 
…y-project#33196)

Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
Signed-off-by: elliottower <elliot@elliottower.com>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
@woshiyyya @ProjectsByJackHe
[AIR] <Part 3> Add LightningPredictor to support batch prediction (ra…
a0b8938 
…y-project#33196)

Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
Signed-off-by: Jack He <jackhe2345@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinvyu justinvyu justinvyu approved these changes

@gjoliver gjoliver gjoliver approved these changes

@amogkam amogkam Awaiting requested review from amogkam

Assignees

@gjoliver gjoliver

@amogkam amogkam

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@woshiyyya @justinvyu @gjoliver @amogkam