Added checkpointing support to Neptune Scale Logger #2
Conversation
Reviewer's GuideThis PR extends NeptuneScaleLogger with automatic model checkpoint and summary logging, updates documentation links and examples, refactors status logging in finalize, and adds tests covering these new features. Sequence Diagram: Model Checkpoint Logging in NeptuneScaleLoggersequenceDiagram
participant T as Trainer
participant NSL as NeptuneScaleLogger
participant MCC as ModelCheckpoint
participant NR as NeptuneRun
T ->> NSL: after_save_checkpoint(checkpoint_callback: ModelCheckpoint)
NSL ->> NSL: if not self._log_model_checkpoints: return
NSL ->> MCC: Access last_model_path
opt Has last_model_path
NSL ->> NSL: model_last_name = _get_full_model_name(MCC.last_model_path, MCC)
NSL ->> NR: run.log_configs({f"{NSL._prefix}/model/checkpoints/{model_last_name}": MCC.last_model_path})
end
NSL ->> MCC: Access best_k_models
loop For each path_key in MCC.best_k_models
NSL ->> NSL: model_name = _get_full_model_name(path_key, MCC)
NSL ->> NR: run.log_configs({f"{NSL._prefix}/model/checkpoints/{model_name}": path_key})
end
NSL ->> MCC: Access best_model_path
opt Has best_model_path
NSL ->> NR: run.log_configs({f"{NSL._prefix}/model/best_model_path": MCC.best_model_path})
NSL ->> NSL: model_name = _get_full_model_name(MCC.best_model_path, MCC)
NSL ->> NR: run.log_configs({f"{NSL._prefix}/model/checkpoints/{model_name}": MCC.best_model_path})
end
NSL ->> MCC: Access best_model_score
opt Has best_model_score
NSL ->> NR: run.log_configs({f"{NSL._prefix}/model/best_model_score": float(MCC.best_model_score)})
end
Sequence Diagram: Model Summary Logging in NeptuneScaleLoggersequenceDiagram
participant Caller
participant NSL as NeptuneScaleLogger
participant MS as ModelSummary
participant NR as NeptuneRun
Caller ->> NSL: log_model_summary(model, max_depth)
NSL ->> MS: Create ModelSummary(model, max_depth)
MS -->> NSL: model_summary_instance
NSL ->> NSL: model_str = str(model_summary_instance)
NSL ->> NR: run.assign_files({f"{NSL._prefix}/model/summary": File(source=model_str.encode("utf-8"))})
Sequence Diagram: Updated Status Finalization in NeptuneScaleLoggersequenceDiagram
participant T as Trainer
participant NSL as NeptuneScaleLogger
participant NR as NeptuneRun
T ->> NSL: finalize(status: str)
NSL ->> NR: run.log_configs({f"{NSL._prefix}/status": status})
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds checkpointing and model summary support to NeptuneScaleLogger, updates status handling, and refreshes documentation links.
- Implements log_model_summary to log model summaries as text files instead of issuing a warning.
- Adds functionality in after_save_checkpoint to automatically log model checkpoints.
- Updates documentation links and test cases to reflect Neptune Scale support.
Reviewed Changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/tests_pytorch/loggers/test_neptune.py | Revised tests for log_model_summary and after_save_checkpoint to validate new assignment calls (though the tests still assert setitem, which may need updating). |
| src/pytorch_lightning/README.md | Updated documentation sections to distinguish between Neptune 2.x and 3.x. |
| src/lightning/pytorch/loggers/neptune.py | Reworked NeptuneScaleLogger implementation to support checkpointing and model summary logging along with refreshed docs and improved API calls. |
Files not reviewed (3)
- docs/source-pytorch/extensions/logging.rst: Language not supported
- docs/source-pytorch/visualize/supported_exp_managers.rst: Language not supported
- requirements/pytorch/loggers.info: Language not supported
Comments suppressed due to low confidence (2)
src/lightning/pytorch/loggers/neptune.py:1127
- The return type annotation 'set[None]' appears to be incorrect; consider using 'set[str]' to accurately reflect the expected return values from this function.
def _get_full_model_names_from_exp_structure(cls, exp_structure: dict[str, Any], namespace: str) -> set[None]:
tests/tests_pytorch/loggers/test_neptune.py:492
- The test assertion still checks for a setitem call, but the new implementation of log_model_summary uses 'assign_files'. Update the test to verify that 'assign_files' is called with the expected arguments.
run_instance_mock.__setitem__.assert_called_once_with(model_summary_key, File)
⛈️ Required checks status: Has failure 🔴
Groups summary🟡 pytorch_lightning: Tests workflow
These checks are required after the changes to 🟡 pytorch_lightning: Azure GPU
These checks are required after the changes to 🟡 pytorch_lightning: Benchmarks
These checks are required after the changes to 🔴 pytorch_lightning: Docs
These checks are required after the changes to 🔴 pytorch_lightning: Docker
These checks are required after the changes to 🔴 mypy
These checks are required after the changes to 🟡 install
These checks are required after the changes to Thank you for your contribution! 💜
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @SiddhantSadangi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 5 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @SiddhantSadangi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey @SiddhantSadangi - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
-
Project placeholder should be a string literal. (link)
-
Update the tests for log_model_summary and after_save_checkpoint to assert run.assign_files and run.log_configs calls (instead of setitem/getitem or upload) since the implementation now uses those APIs.
-
Verify that neptune_scale.types.File(source=bytes, mime_type) is the correct way to upload text summaries—consider using a File.from_content utility if raw bytes aren’t supported by the File constructor.
-
Either remove or implement the commented‐out
dellogic in after_save_checkpoint oncedelsupport is available, to avoid accumulating dead code.
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 1 other issue
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
… of uploading checkpoints
|
@sourcery-ai review |
There was a problem hiding this comment.
Pull Request Overview
This PR adds checkpointing support and model summary logging to the Neptune Scale Logger while updating documentation links and extending tests. Key changes include updating tests for model summary and checkpoint logging, modifying documentation references for Neptune/Neptune Scale, and refactoring logger methods to support the new features.
Reviewed Changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/tests_pytorch/loggers/test_neptune.py | Updated tests to assert proper logging in model summary and checkpointing. |
| src/pytorch_lightning/README.md | Updated documentation links and labels to differentiate Neptune versions. |
| src/lightning/pytorch/loggers/neptune.py | Implemented log_model_summary, improved checkpoint logging, and updated docstrings and API behavior. |
Files not reviewed (3)
- docs/source-pytorch/extensions/logging.rst: Language not supported
- docs/source-pytorch/visualize/supported_exp_managers.rst: Language not supported
- requirements/pytorch/loggers.info: Language not supported
Comments suppressed due to low confidence (1)
src/lightning/pytorch/loggers/neptune.py:1043
- ModelSummary is used without an explicit import, which could raise a NameError. Please add the appropriate import for ModelSummary.
model_str = str(ModelSummary(model=model, max_depth=max_depth))
…olders in documentation
There was a problem hiding this comment.
Hey @SiddhantSadangi - I've reviewed your changes - here's some feedback:
- Update the docstring note in the Log model checkpoint paths section to reflect that
save_lastandsave_top_kare now supported, or clearly specify which options remain unsupported. - Ensure
import osis present at the top of the file since_get_full_model_nameusesos.pathandos.sep. - Add a test for
log_model_checkpoints=Falseto verify thatafter_save_checkpointdoes not log checkpoint paths when the flag is disabled.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary by Sourcery
Add checkpointing and model summary support to NeptuneScaleLogger, improve finalize status handling, and refresh documentation links for NeptuneLogger and NeptuneScaleLogger.
New Features:
Enhancements:
Documentation:
Tests: