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.

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

Skip tests that cause CLI argparse errors on Python 3.11.9 #19756

Merged
merged 9 commits into from
Apr 11, 2024

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Apr 10, 2024

What does this PR do?

Python 3.11.9 was released recently and is picked up by our CI. It had a breaking change that jsonargparse relied on.

https://github.com/Lightning-AI/pytorch-lightning/actions/runs/8633090509/job/23665307586?pr=19755

_________________ test_wandb_logger_cli_integration[True-True] _________________

log_model = 'True', expected = True, wandb_mock = <module 'wandb'>
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x15a365d50>
tmp_path = PosixPath('/private/var/folders/n2/pt_35rc53tdgkld9531s2tfh0000gn/T/pytest-of-runner/pytest-0/test_wandb_logger_cli_integrat2')

    @pytest.mark.parametrize(("log_model", "expected"), [("True", True), ("False", False), ("all", "all")])
    def test_wandb_logger_cli_integration(log_model, expected, wandb_mock, monkeypatch, tmp_path):
        """Test that the WandbLogger can be used with the LightningCLI."""
        monkeypatch.chdir(tmp_path)
    
        class InspectParsedCLI(LightningCLI):
            def before_instantiate_classes(self):
                assert self.config.trainer.logger.init_args.log_model == expected
    
        # Create a config file with the log_model parameter set. This seems necessary to be able
        # to set the init_args parameter of the logger on the CLI later on.
        input_config = {
            "trainer": {
                "logger": {
                    "class_path": "pytorch_lightning.loggers.wandb.WandbLogger",
                    "init_args": {"log_model": log_model},
                },
            }
        }
        config_path = "config.yaml"
        with open(config_path, "w") as f:
            f.write(yaml.dump(input_config))
    
        # Test case 1: Set the log_model parameter only via the config file.
        with mock.patch("sys.argv", ["any.py", "--config", config_path]):
            InspectParsedCLI(BoringModel, run=False, save_config_callback=None)
    
        # Test case 2: Overwrite the log_model parameter via the command line.
        wandb_cli_arg = f"--trainer.logger.init_args.log_model={log_model}"
    
        with mock.patch("sys.argv", ["any.py", "--config", config_path, wandb_cli_arg]):
>           InspectParsedCLI(BoringModel, run=False, save_config_callback=None)

/Users/runner/work/pytorch-lightning/pytorch-lightning/tests/tests_pytorch/loggers/test_wandb.py:582: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/pytorch_lightning/cli.py:383: in __init__
    self.parse_arguments(self.parser, args)
/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/pytorch_lightning/cli.py:534: in parse_arguments
    self.config = parser.parse_args(args)
/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/jsonargparse/_deprecated.py:124: in patched_parse
    cfg = parse_method(*args, _skip_check=_skip_check, **kwargs)
/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/jsonargparse/_core.py:390: in parse_args
    cfg, unk = self.parse_known_args(args=args, namespace=cfg)
/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/jsonargparse/_core.py:261: in parse_known_args
    namespace, args = self._parse_known_args(args, namespace)
/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/argparse.py:2128: in _parse_known_args
    start_index = consume_optional(start_index)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

start_index = 2

    def consume_optional(start_index):
    
        # get the optional identified at this index
        option_tuple = option_string_indices[start_index]
>       action, option_string, sep, explicit_arg = option_tuple
E       ValueError: not enough values to unpack (expected 4, got 3)

/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/argparse.py:1990: ValueError

We can undo this PR once jsonargparse has released the fix:
omni-us/jsonargparse#484


📚 Documentation preview 📚: https://pytorch-lightning--19756.org.readthedocs.build/en/19756/

cc @Borda

@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Apr 10, 2024
@awaelchli awaelchli added tests and removed pl Generic label for PyTorch Lightning package labels Apr 10, 2024
Copy link
Contributor

github-actions bot commented Apr 10, 2024

⚡ Required checks status: All passing 🟢

Groups summary

🟢 pytorch_lightning: Tests workflow
Check ID Status
pl-cpu (macOS-11, lightning, 3.8, 1.13, oldest) success
pl-cpu (macOS-11, lightning, 3.10, 1.13) success
pl-cpu (macOS-11, lightning, 3.10, 2.1) success
pl-cpu (macOS-11, lightning, 3.10, 2.2) success
pl-cpu (ubuntu-20.04, lightning, 3.8, 1.13, oldest) success
pl-cpu (ubuntu-20.04, lightning, 3.10, 1.13) success
pl-cpu (ubuntu-20.04, lightning, 3.10, 2.1) success
pl-cpu (ubuntu-20.04, lightning, 3.10, 2.2) success
pl-cpu (windows-2022, lightning, 3.8, 1.13, oldest) success
pl-cpu (windows-2022, lightning, 3.10, 1.13) success
pl-cpu (windows-2022, lightning, 3.10, 2.1) success
pl-cpu (windows-2022, lightning, 3.10, 2.2) success
pl-cpu (macOS-11, pytorch, 3.8, 1.13) success
pl-cpu (ubuntu-20.04, pytorch, 3.8, 1.13) success
pl-cpu (windows-2022, pytorch, 3.8, 1.13) success
pl-cpu (macOS-12, pytorch, 3.11, 2.0) success
pl-cpu (macOS-12, pytorch, 3.11, 2.1) success
pl-cpu (ubuntu-22.04, pytorch, 3.11, 2.0) success
pl-cpu (ubuntu-22.04, pytorch, 3.11, 2.1) success
pl-cpu (windows-2022, pytorch, 3.11, 2.0) success
pl-cpu (windows-2022, pytorch, 3.11, 2.1) success

These checks are required after the changes to tests/tests_pytorch/loggers/conftest.py, tests/tests_pytorch/loggers/test_mlflow.py, tests/tests_pytorch/loggers/test_wandb.py, tests/tests_pytorch/test_cli.py.

🟢 pytorch_lightning: Azure GPU
Check ID Status
pytorch-lightning (GPUs) (testing Lightning | latest) success
pytorch-lightning (GPUs) (testing PyTorch | latest) success

These checks are required after the changes to tests/tests_pytorch/loggers/conftest.py, tests/tests_pytorch/loggers/test_mlflow.py, tests/tests_pytorch/loggers/test_wandb.py, tests/tests_pytorch/test_cli.py.


Thank you for your contribution! 💜

Note
This comment is automatically generated and updates for 60 minutes every 180 seconds. If you have any other questions, contact carmocca for help.

@awaelchli awaelchli changed the title Skip CLI argparse errors on Python 3.11 Skip tests that cause CLI argparse errors on Python 3.11 Apr 10, 2024
@awaelchli awaelchli changed the title Skip tests that cause CLI argparse errors on Python 3.11 Skip tests that cause CLI argparse errors on Python 3.11.9 Apr 10, 2024
@awaelchli awaelchli added 3rd party Related to a 3rd-party bug Something isn't working labels Apr 10, 2024
@awaelchli awaelchli added this to the 2.2.x milestone Apr 10, 2024
@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Apr 10, 2024
@awaelchli awaelchli requested a review from Borda April 10, 2024 23:39
Copy link

codecov bot commented Apr 11, 2024

Codecov Report

Merging #19756 (5deabc9) into master (76b691d) will decrease coverage by 29%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #19756      +/-   ##
==========================================
- Coverage      82%      53%     -29%     
==========================================
  Files         424      416       -8     
  Lines       34919    34766     -153     
==========================================
- Hits        28628    18445   -10183     
- Misses       6291    16321   +10030     

@mergify mergify bot added the ready PRs ready to be merged label Apr 11, 2024
@awaelchli awaelchli merged commit 316cc71 into master Apr 11, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party Related to a 3rd-party bug Something isn't working pl Generic label for PyTorch Lightning package ready PRs ready to be merged tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants