Skip to content
This repository was archived by the owner on Nov 3, 2023. It is now read-only.

LightningCLI and RayPlugin compatibility#164

Merged
amogkam merged 3 commits intoray-project:mainfrom
mauvilsa:cli-ray-plugin
Sep 7, 2022
Merged

LightningCLI and RayPlugin compatibility#164
amogkam merged 3 commits intoray-project:mainfrom
mauvilsa:cli-ray-plugin

Conversation

@mauvilsa
Copy link
Copy Markdown
Contributor

@mauvilsa mauvilsa commented Jun 22, 2022 •
edited
Loading

Proper fix for #151 removing what #154 added.

cc @amogkam

@mauvilsa mauvilsa marked this pull request as ready for review June 22, 2022 04:43
@amogkam
Copy link
Copy Markdown
Collaborator

amogkam commented Jun 22, 2022 •
edited
Loading

Thanks for the contribution @mauvilsa!

Could you add this test below this line https://github.com/ray-project/ray_lightning/blob/main/.github/workflows/test.yaml#L133 so that it gets run in CI?

Also seems like there are some lint failures. Could you run format.sh to fix those?

@mauvilsa
Copy link
Copy Markdown
Contributor Author

mauvilsa commented Jun 23, 2022 •
edited
Loading

Thanks for the contribution @mauvilsa!

Could you add this test below this line https://github.com/ray-project/ray_lightning/blob/main/.github/workflows/test.yaml#L133 so that it gets run in CI?

Also seems like there are some lint failures. Could you run format.sh to fix those?

@amogkam done.

@mauvilsa
Copy link
Copy Markdown
Contributor Author

mauvilsa commented Aug 30, 2022

@amogkam I addressed your comment long ago. So long that the removal of the workaround was already done in 299a776. However, adding the unit test still make sense.

@amogkam
Copy link
Copy Markdown
Collaborator

amogkam commented Aug 30, 2022

Thanks @mauvilsa and apologies for the delay! Just re-triggered the CI again.

@amogkam amogkam self-assigned this Aug 30, 2022
@mauvilsa
Copy link
Copy Markdown
Contributor Author

mauvilsa commented Aug 31, 2022

I committed fix the unit test for the rename from RayPlugin to RayStrategy. @amogkam please re-trigger the CI.

amogkam
amogkam approved these changes Sep 7, 2022
View reviewed changes
ray_lightning/tests/test_lightning_cli.py Outdated
import pytest
from importlib.util import find_spec
from pytorch_lightning.utilities.cli import LightningCLI
from ray_lightning import RayPlugin
Copy link
Copy Markdown
Collaborator

@amogkam amogkam Aug 30, 2022

Choose a reason for hiding this comment

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

Suggested change
from ray_lightning import RayPlugin
from ray_lightning import RayStrategy

ray_lightning/tests/test_lightning_cli.py Outdated

@pytest.mark.skipif(
not find_spec("jsonargparse"), reason="jsonargparse required")
def test_lightning_cli_rayplugin_instantiation():
Copy link
Copy Markdown
Collaborator

@amogkam amogkam Aug 30, 2022

Choose a reason for hiding this comment

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

Suggested change
def test_lightning_cli_rayplugin_instantiation():
def test_lightning_cli_raystrategy_instantiation():

ray_lightning/tests/test_lightning_cli.py Outdated
with mock.patch("sys.argv", ["any.py"] + cli_args):
cli = LightningCLI(BoringModel, run=False)

assert isinstance(cli.config_init["trainer"]["plugins"], RayPlugin)
Copy link
Copy Markdown
Collaborator

@amogkam amogkam Aug 30, 2022

Choose a reason for hiding this comment

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

Suggested change
assert isinstance(cli.config_init["trainer"]["plugins"], RayPlugin)
assert isinstance(cli.config_init["trainer"]["plugins"], RayStrategy)

@amogkam amogkam merged commit d0be22e into ray-project:main Sep 7, 2022
@mauvilsa mauvilsa deleted the cli-ray-plugin branch September 7, 2022 09:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@amogkam amogkam amogkam approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@amogkam amogkam

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mauvilsa @amogkam