-
Notifications
You must be signed in to change notification settings - Fork 0
Ss/neptune scale integration #1
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
Conversation
Reviewer's Guide by SourceryThis pull request introduces the Sequence diagram for logging metrics with NeptuneScaleLoggersequenceDiagram
participant LightningModule
participant NeptuneScaleLogger
participant Neptune Run
LightningModule->>NeptuneScaleLogger: log_metrics(metrics, step)
activate NeptuneScaleLogger
NeptuneScaleLogger->>Neptune Run: log_metrics(metrics, step)
activate Neptune Run
Neptune Run-->>NeptuneScaleLogger:
deactivate Neptune Run
NeptuneScaleLogger-->>LightningModule:
deactivate NeptuneScaleLogger
Sequence diagram for logging hyperparameters with NeptuneScaleLoggersequenceDiagram
participant LightningModule
participant NeptuneScaleLogger
participant Neptune Run
LightningModule->>NeptuneScaleLogger: log_hyperparams(params)
activate NeptuneScaleLogger
NeptuneScaleLogger->>Neptune Run: log_configs(params)
activate Neptune Run
Neptune Run-->>NeptuneScaleLogger:
deactivate Neptune Run
NeptuneScaleLogger-->>LightningModule:
deactivate NeptuneScaleLogger
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @SiddhantSadangi - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Method _get_full_model_name immediately returns None, making the rest of its code unreachable. (link)
Overall Comments:
- Consider adding a test case that initializes
NeptuneScaleLogger
with all possible arguments to ensure proper handling. - The
_get_full_model_name
method inNeptuneScaleLogger
always returns None; is this intentional?
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 3 other issues
- 🟢 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.
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @SiddhantSadangi - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a test case that initializes
NeptuneScaleLogger
with all possible arguments to ensure compatibility. - The
_get_full_model_name
method inNeptuneScaleLogger
always returnsNone
; is this intentional?
Here's what I looked at during the review
- 🟡 General issues: 5 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.
) | ||
trainer = Trainer(logger=neptune_scale_logger) | ||
|
||
Access the Neptune Scale logger from any function (except the LightningModule *init*) to use its API for tracking advanced artifacts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Clarify logger vs. run
The documentation should refer to accessing the Neptune Scale run, not the logger. The logger is passed to the Trainer, while the run is obtained via logger.experiment
.
neptune_scale_logger.log_metrics(data={"path/to/metadata": metadata}, step=step) | ||
neptune_scale_logger.log_configs(data={"path/to/config": config}) | ||
|
||
Here's the full documentation for the :class:`~lightning.pytorch.loggers.NeptuneScaleLogger`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Missing link to full documentation
The documentation promises a link to the full documentation for the NeptuneScaleLogger
but doesn't provide one. Please add the appropriate link.
What does this PR do?
Fixes #<issue_number>
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Reviewer checklist
Summary by Sourcery
Add support for Neptune Scale logger in PyTorch Lightning
New Features:
Enhancements:
Documentation:
Tests:
Chores: