Merge branch 'master' into deepspeed_mics_init

hehepig4 · web-flow · commit 50a0af7e08ab · 2024-12-19T17:17:48.000+08:00
diff --git a/.github/workflows/_build-packages.yml b/.github/workflows/_build-packages.yml
@@ -19,38 +19,16 @@ defaults:
     shell: bash
 
 jobs:
-  init:
-    runs-on: ubuntu-22.04
-    steps:
-      - uses: actions/checkout@v4
-      - run: |
-          mkdir dist && touch dist/.placeholder
-      - name: Keep artifact
-        id: keep-artifact
-        run: python -c "print('DAYS=' + str(5 if '${{ github.event_name }}'.startswith('pull_request') else 0))" >> $GITHUB_OUTPUT
-      - uses: actions/upload-artifact@v3
-        with:
-          name: ${{ inputs.artifact-name }}
-          path: dist
-          retention-days: ${{ steps.keep-artifact.outputs.DAYS }}
-          include-hidden-files: true
-
   build-packages:
-    needs: init
     runs-on: ubuntu-22.04
     strategy:
-      max-parallel: 1 # run sequential to prevent download/upload collisions
       matrix:
         pkg-name: ${{ fromJSON(inputs.pkg-names) }}
     steps:
       - uses: actions/checkout@v4
-      - uses: actions/download-artifact@v3
-        with:
-          name: ${{ inputs.artifact-name }}
-          path: pypi
       - uses: actions/setup-python@v5
         with:
-          python-version: 3.9
+          python-version: "3.x"
 
       - run: python -c "print('NB_DIRS=' + str(2 if '${{ matrix.pkg-name }}' == 'pytorch' else 1))" >> $GITHUB_ENV
       - name: Build & check package
@@ -60,11 +38,33 @@ jobs:
           nb-dirs: ${{ env.NB_DIRS }}
 
       - run: |
-          mkdir pypi/${{ matrix.pkg-name }}
+          mkdir -p pypi/${{ matrix.pkg-name }}
           cp dist/* pypi/${{ matrix.pkg-name }}/
 
-      - uses: actions/upload-artifact@v3
+      - uses: actions/upload-artifact@v4
+        with:
+          name: ${{ inputs.artifact-name }}-${{ matrix.pkg-name }}
+          path: pypi
+          retention-days: 1
+
+  merge-artifacts:
+    needs: build-packages
+    runs-on: ubuntu-22.04
+    steps:
+      - uses: actions/download-artifact@v4
+        with: # download all build artifacts
+          pattern: ${{ inputs.artifact-name }}-*
+          merge-multiple: true
+          path: pypi
+      - run: |
+          sudo apt-get install -y tree
+          tree pypi
+
+      - name: Keep artifact
+        run: python -c "print('DAYS=' + str(5 if '${{ github.event_name }}'.startswith('pull_request') else 0))" >> $GITHUB_ENV
+      - uses: actions/upload-artifact@v4
         with:
           name: ${{ inputs.artifact-name }}
           path: pypi
-          include-hidden-files: true
+          retention-days: ${{ env.DAYS }}
+          if-no-files-found: error
diff --git a/.github/workflows/_legacy-checkpoints.yml b/.github/workflows/_legacy-checkpoints.yml
@@ -104,7 +104,7 @@ jobs:
           python -c "print('AWS_RUN=' + str('' if '${{inputs.push_to_s3}}' == 'true' else '--dryrun'))" >> $GITHUB_ENV
 
       - name: Upload checkpoints to GitHub Actions artifact
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: checkpoints-${{ github.sha }}
           path: ${{ env.LEGACY_FOLDER }}/checkpoints/
diff --git a/.github/workflows/ci-pkg-install.yml b/.github/workflows/ci-pkg-install.yml
@@ -50,7 +50,7 @@ jobs:
       - uses: actions/setup-python@v5
         with:
           python-version: ${{ matrix.python-version }}
-      - uses: actions/download-artifact@v3
+      - uses: actions/download-artifact@v4
         with:
           name: dist-packages-${{ github.sha }}
           path: dist
diff --git a/.github/workflows/docs-build.yml b/.github/workflows/docs-build.yml
@@ -129,7 +129,7 @@ jobs:
         run: echo "ARTIFACT_DAYS=7" >> $GITHUB_ENV
       - name: Upload built docs
         if: ${{ matrix.target == 'html' }}
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: docs-${{ matrix.pkg-name }}-${{ github.sha }}
           path: docs/build/html/
@@ -158,7 +158,7 @@ jobs:
       # use input if dispatch or git tag
       VERSION: ${{ inputs.version || github.ref_name }}
     steps:
-      - uses: actions/download-artifact@v3
+      - uses: actions/download-artifact@v4
         with:
           name: docs-${{ matrix.pkg-name }}-${{ github.sha }}
           path: docs/build/html/
diff --git a/.github/workflows/release-pkg.yml b/.github/workflows/release-pkg.yml
@@ -38,7 +38,7 @@ jobs:
     if: github.event_name == 'release'
     steps:
       - uses: actions/checkout@v4
-      - uses: actions/download-artifact@v3
+      - uses: actions/download-artifact@v4
         with:
           name: dist-packages-${{ github.sha }}
           path: dist
@@ -140,7 +140,7 @@ jobs:
         name: ["FABRIC", "PYTORCH", "LIGHTNING"]
     steps:
       - uses: actions/checkout@v4 # needed for local action below
-      - uses: actions/download-artifact@v3
+      - uses: actions/download-artifact@v4
         with:
           name: dist-packages-${{ github.sha }}
           path: dist
@@ -165,7 +165,7 @@ jobs:
         name: ["FABRIC", "PYTORCH", "LIGHTNING"]
     steps:
       - uses: actions/checkout@v4 # needed for local action below
-      - uses: actions/download-artifact@v3
+      - uses: actions/download-artifact@v4
         with:
           name: dist-packages-${{ github.sha }}
           path: dist
diff --git a/docs/source-pytorch/conf.py b/docs/source-pytorch/conf.py
@@ -643,6 +643,7 @@ def package_list_from_file(file):
     r"starter/installation.html$",
     r"^../common/trainer.html#trainer-flags$",
     "https://deepgenerativemodels.github.io/assets/slides/cs236_lecture11.pdf",
+    "https://developer.habana.ai", # returns 403 error but redirects to intel.com documentation
     "https://www.intel.com/content/www/us/en/products/docs/processors/what-is-a-gpu.html",
     "https://www.microsoft.com/en-us/research/blog/zero-infinity-and-deepspeed-unlocking-unprecedented-model-scale-for-deep-learning-training/",  # noqa: E501
     "https://stackoverflow.com/questions/66640705/how-can-i-install-grpcio-on-an-apple-m1-silicon-laptop",
diff --git a/src/lightning/fabric/loggers/tensorboard.py b/src/lightning/fabric/loggers/tensorboard.py
@@ -220,15 +220,19 @@ def log_metrics(self, metrics: Mapping[str, float], step: Optional[int] = None)
     @override
     @rank_zero_only
     def log_hyperparams(
-        self, params: Union[dict[str, Any], Namespace], metrics: Optional[dict[str, Any]] = None
+        self,
+        params: Union[dict[str, Any], Namespace],
+        metrics: Optional[dict[str, Any]] = None,
+        step: Optional[int] = None,
     ) -> None:
         """Record hyperparameters. TensorBoard logs with and without saved hyperparameters are incompatible, the
         hyperparameters are then not displayed in the TensorBoard. Please delete or move the previously saved logs to
         display the new ones with hyperparameters.
 
         Args:
-            params: a dictionary-like container with the hyperparameters
+            params: A dictionary-like container with the hyperparameters
             metrics: Dictionary with metric names as keys and measured quantities as values
+            step: Optional global step number for the logged metrics
 
         """
         params = _convert_params(params)
@@ -244,7 +248,7 @@ def log_hyperparams(
             metrics = {"hp_metric": metrics}
 
         if metrics:
-            self.log_metrics(metrics, 0)
+            self.log_metrics(metrics, step)
 
             if _TENSORBOARD_AVAILABLE:
                 from torch.utils.tensorboard.summary import hparams
@@ -253,9 +257,9 @@ def log_hyperparams(
 
             exp, ssi, sei = hparams(params, metrics)
             writer = self.experiment._get_file_writer()
-            writer.add_summary(exp)
-            writer.add_summary(ssi)
-            writer.add_summary(sei)
+            writer.add_summary(exp, step)
+            writer.add_summary(ssi, step)
+            writer.add_summary(sei, step)
 
     @override
     @rank_zero_only
diff --git a/src/lightning/pytorch/CHANGELOG.md b/src/lightning/pytorch/CHANGELOG.md
@@ -5,6 +5,21 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 
+## [unreleased] - YYYY-MM-DD
+
+### Added
+
+### Changed
+
+- Merging of hparams when logging now ignores parameter names that begin with underscore `_` ([#20221](https://github.com/Lightning-AI/pytorch-lightning/pull/20221))
+
+### Removed
+
+### Fixed
+
+- Fix LightningCLI failing when both module and data module save hyperparameters due to conflicting internal `_class_path` parameter ([#20221](https://github.com/Lightning-AI/pytorch-lightning/pull/20221))
+
+
 ## [2.4.0] - 2024-08-06
 
 ### Added
diff --git a/src/lightning/pytorch/loggers/tensorboard.py b/src/lightning/pytorch/loggers/tensorboard.py
@@ -153,15 +153,19 @@ def save_dir(self) -> str:
     @override
     @rank_zero_only
     def log_hyperparams(
-        self, params: Union[dict[str, Any], Namespace], metrics: Optional[dict[str, Any]] = None
+        self,
+        params: Union[dict[str, Any], Namespace],
+        metrics: Optional[dict[str, Any]] = None,
+        step: Optional[int] = None,
     ) -> None:
         """Record hyperparameters. TensorBoard logs with and without saved hyperparameters are incompatible, the
         hyperparameters are then not displayed in the TensorBoard. Please delete or move the previously saved logs to
         display the new ones with hyperparameters.
 
         Args:
-            params: a dictionary-like container with the hyperparameters
+            params: A dictionary-like container with the hyperparameters
             metrics: Dictionary with metric names as keys and measured quantities as values
+            step: Optional global step number for the logged metrics
 
         """
         if _OMEGACONF_AVAILABLE:
@@ -175,7 +179,7 @@ def log_hyperparams(
         else:
             self.hparams.update(params)
 
-        return super().log_hyperparams(params=params, metrics=metrics)
+        return super().log_hyperparams(params=params, metrics=metrics, step=step)
 
     @override
     @rank_zero_only
diff --git a/src/lightning/pytorch/loggers/utilities.py b/src/lightning/pytorch/loggers/utilities.py
@@ -69,6 +69,9 @@ def _log_hyperparams(trainer: "pl.Trainer") -> None:
         lightning_hparams = pl_module.hparams_initial
         inconsistent_keys = []
         for key in lightning_hparams.keys() & datamodule_hparams.keys():
+            if key == "_class_path":
+                # Skip LightningCLI's internal hparam
+                continue
             lm_val, dm_val = lightning_hparams[key], datamodule_hparams[key]
             if (
                 type(lm_val) != type(dm_val)
@@ -88,6 +91,10 @@ def _log_hyperparams(trainer: "pl.Trainer") -> None:
     elif datamodule_log_hyperparams:
         hparams_initial = trainer.datamodule.hparams_initial
 
+    # Don't log LightningCLI's internal hparam
+    if hparams_initial is not None:
+        hparams_initial = {k: v for k, v in hparams_initial.items() if k != "_class_path"}
+
     for logger in trainer.loggers:
         if hparams_initial is not None:
             logger.log_hyperparams(hparams_initial)
diff --git a/src/version.info b/src/version.info
@@ -1 +1 @@
-2.5.0.dev
+2.5.0rc0
diff --git a/tests/tests_pytorch/test_cli.py b/tests/tests_pytorch/test_cli.py
@@ -973,6 +973,29 @@ def test_lightning_cli_save_hyperparameters_untyped_module(cleandir):
     assert model.kwargs == {"x": 1}
 
 
+class TestDataSaveHparams(BoringDataModule):
+    def __init__(self, batch_size: int = 32, num_workers: int = 4):
+        super().__init__()
+        self.save_hyperparameters()
+        self.batch_size = batch_size
+        self.num_workers = num_workers
+
+
+def test_lightning_cli_save_hyperparameters_merge(cleandir):
+    config = {
+        "model": {
+            "class_path": f"{__name__}.TestModelSaveHparams",
+        },
+        "data": {
+            "class_path": f"{__name__}.TestDataSaveHparams",
+        },
+    }
+    with mock.patch("sys.argv", ["any.py", "fit", f"--config={json.dumps(config)}", "--trainer.max_epochs=1"]):
+        cli = LightningCLI(auto_configure_optimizers=False)
+    assert set(cli.model.hparams) == {"optimizer", "scheduler", "activation", "_instantiator", "_class_path"}
+    assert set(cli.datamodule.hparams) == {"batch_size", "num_workers", "_instantiator", "_class_path"}
+
+
 @pytest.mark.parametrize("fn", [fn.value for fn in TrainerFn])
 def test_lightning_cli_trainer_fn(fn):
     class TestCLI(LightningCLI):
diff --git a/tests/tests_pytorch/trainer/logging_/test_eval_loop_logging.py b/tests/tests_pytorch/trainer/logging_/test_eval_loop_logging.py
@@ -544,7 +544,7 @@ def test_step(self, batch, batch_idx):
         "valid_loss_1",
     }
     assert mock_log_metrics.mock_calls == [
-        call({"hp_metric": -1}, 0),
+        call({"hp_metric": -1}, None),
         call(metrics={"train_loss": ANY, "epoch": 0}, step=0),
         call(metrics={"valid_loss_0_step": ANY, "valid_loss_2": ANY}, step=0),
         call(metrics={"valid_loss_0_step": ANY, "valid_loss_2": ANY}, step=1),
