From 84053beddaf5a87b4a3e02763ff505dc547bd348 Mon Sep 17 00:00:00 2001 From: Mr WhatZitTooYa Date: Mon, 14 Oct 2024 16:53:58 -0400 Subject: [PATCH 1/4] Added first test that shows mentioned deficites --- tests/tests_pytorch/test_cli.py | 38 +++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/tests_pytorch/test_cli.py b/tests/tests_pytorch/test_cli.py index 56b58d4d157a1..6fded08f332f5 100644 --- a/tests/tests_pytorch/test_cli.py +++ b/tests/tests_pytorch/test_cli.py @@ -597,6 +597,44 @@ def add_arguments_to_parser(self, parser): assert cli.model.num_classes == 5 +def test_lightning_cli_link_arguments_init(): + # Will not work without init_args ("--data.init_args.batch_size=12") + class MyLightningCLI(LightningCLI): + def add_arguments_to_parser(self, parser): + parser.link_arguments("data.batch_size", "model.init_args.batch_size") + + cli_args = [ + "--data=tests_pytorch.test_cli.BoringDataModuleBatchSizeAndClasses", + "--model=tests_pytorch.test_cli.BoringModelRequiredClasses", + "--data.init_args.batch_size=12", + "--model.init_args.num_classes=5", + ] + + with mock.patch("sys.argv", ["any.py"] + cli_args): + cli = MyLightningCLI(run=False) + + assert cli.datamodule.batch_size == 12 + + # Will work without init_args ("--data.batch_size=12") + class MyLightningCLI(LightningCLI): + def add_arguments_to_parser(self, parser): + pass + + cli_args = [ + "--data=tests_pytorch.test_cli.BoringDataModuleBatchSizeAndClasses", + "--model=tests_pytorch.test_cli.BoringModelRequiredClasses", + "--data.batch_size=12", + "--model.num_classes=12", + ] + + with mock.patch("sys.argv", ["any.py"] + cli_args): + cli = MyLightningCLI(run=False) + + print(cli.config) + + assert cli.datamodule.batch_size == 12 + + class EarlyExitTestModel(BoringModel): def on_fit_start(self): raise MisconfigurationException("Error on fit start") From 735b62fecb4efeec3d1f771995bf4be47ec6ae00 Mon Sep 17 00:00:00 2001 From: Mr WhatZitTooYa Date: Sun, 27 Oct 2024 12:51:20 -0400 Subject: [PATCH 2/4] Implemented test for cli It shows the inconsitency that is required to cli arguments depending on how you set up the model and data module --- tests/tests_pytorch/test_cli.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/tests/tests_pytorch/test_cli.py b/tests/tests_pytorch/test_cli.py index 6fded08f332f5..4a57e0cc4c89d 100644 --- a/tests/tests_pytorch/test_cli.py +++ b/tests/tests_pytorch/test_cli.py @@ -597,42 +597,47 @@ def add_arguments_to_parser(self, parser): assert cli.model.num_classes == 5 +# Notes: +# - if variable is class attribute, it will only be present after instantiation -> apply_on="instantiate" +# If you link argumetns and then try to pass them additionally, it will raise an error + + def test_lightning_cli_link_arguments_init(): # Will not work without init_args ("--data.init_args.batch_size=12") class MyLightningCLI(LightningCLI): def add_arguments_to_parser(self, parser): - parser.link_arguments("data.batch_size", "model.init_args.batch_size") + parser.link_arguments("data.batch_size", "model.batch_size") + parser.link_arguments("data.num_classes", "model.num_classes", apply_on="instantiate") cli_args = [ - "--data=tests_pytorch.test_cli.BoringDataModuleBatchSizeAndClasses", - "--model=tests_pytorch.test_cli.BoringModelRequiredClasses", - "--data.init_args.batch_size=12", - "--model.init_args.num_classes=5", + "--data.batch_size=12", ] with mock.patch("sys.argv", ["any.py"] + cli_args): - cli = MyLightningCLI(run=False) + cli = MyLightningCLI(BoringModelRequiredClasses, BoringDataModuleBatchSizeAndClasses, run=False) + assert cli.model.batch_size == 12 assert cli.datamodule.batch_size == 12 + assert cli.model.num_classes == 5 + assert cli.datamodule.num_classes == 5 # Will work without init_args ("--data.batch_size=12") class MyLightningCLI(LightningCLI): def add_arguments_to_parser(self, parser): - pass + parser.link_arguments("data.batch_size", "model.batch_size") + parser.link_arguments("data.num_classes", "model.num_classes", apply_on="instantiate") cli_args = [ "--data=tests_pytorch.test_cli.BoringDataModuleBatchSizeAndClasses", "--model=tests_pytorch.test_cli.BoringModelRequiredClasses", "--data.batch_size=12", - "--model.num_classes=12", ] with mock.patch("sys.argv", ["any.py"] + cli_args): cli = MyLightningCLI(run=False) - print(cli.config) - assert cli.datamodule.batch_size == 12 + assert cli.model.batch_size == 12 class EarlyExitTestModel(BoringModel): From 0dd002429f8bf7949d7f2375ec36155f9c72e972 Mon Sep 17 00:00:00 2001 From: Mr WhatZitTooYa Date: Sat, 9 Nov 2024 11:33:49 -0500 Subject: [PATCH 3/4] Perfomed experiments in passing linked cmd args to model and data. Added two additional findings to the notes in the comment form the findings --- tests/tests_pytorch/test_cli.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/tests_pytorch/test_cli.py b/tests/tests_pytorch/test_cli.py index 4a57e0cc4c89d..0a3b885211f78 100644 --- a/tests/tests_pytorch/test_cli.py +++ b/tests/tests_pytorch/test_cli.py @@ -599,7 +599,11 @@ def add_arguments_to_parser(self, parser): # Notes: # - if variable is class attribute, it will only be present after instantiation -> apply_on="instantiate" -# If you link argumetns and then try to pass them additionally, it will raise an error +# - if you link argumetns and then try to pass them additionally, it will raise an error +# - if you initilize the model from the cmd instead of passing it as an instance to CLI init_args needs to be +# added additionally +# - if you initilize the model from the cmd instead of passing it as an instance to CLI apply on instantiate needs to +# be always true for linked arguments def test_lightning_cli_link_arguments_init(): @@ -624,8 +628,8 @@ def add_arguments_to_parser(self, parser): # Will work without init_args ("--data.batch_size=12") class MyLightningCLI(LightningCLI): def add_arguments_to_parser(self, parser): - parser.link_arguments("data.batch_size", "model.batch_size") - parser.link_arguments("data.num_classes", "model.num_classes", apply_on="instantiate") + parser.link_arguments("data.batch_size", "model.init_args.batch_size", apply_on="instantiate") + parser.link_arguments("data.num_classes", "model.init_args.num_classes", apply_on="instantiate") cli_args = [ "--data=tests_pytorch.test_cli.BoringDataModuleBatchSizeAndClasses", From ecfa137d7e3cf8c897b930dfec019a34a2083602 Mon Sep 17 00:00:00 2001 From: Mr WhatZitTooYa Date: Sat, 9 Nov 2024 14:33:42 -0500 Subject: [PATCH 4/4] Experiments with CLI cmd passing Added temporary debug print statements --- src/lightning/pytorch/cli.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/lightning/pytorch/cli.py b/src/lightning/pytorch/cli.py index 26af335f7be93..9f970f0c22a42 100644 --- a/src/lightning/pytorch/cli.py +++ b/src/lightning/pytorch/cli.py @@ -138,7 +138,10 @@ def add_lightning_class_args( if issubclass(lightning_class, Callback): self.callback_keys.append(nested_key) if subclass_mode: + print("Subclass mode") + print(nested_key) return self.add_subclass_arguments(lightning_class, nested_key, fail_untyped=False, required=required) + print(nested_key) return self.add_class_arguments( lightning_class, nested_key,