[compat] neptune#4
Merged
Merged
Conversation
Summary of ChangesHello @asaiacai, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a crucial compatibility layer to streamline the migration of experiment tracking from Neptune to mlop's Trakkur platform. It allows users to simultaneously log data to both systems during a transition period, ensuring continuity and minimizing disruption to existing workflows. The solution leverages monkeypatching to intercept Neptune API calls, providing a risk-free, gradual migration path without requiring extensive code modifications in user scripts. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive compatibility layer to facilitate migrating from Neptune to mlop, including the core monkeypatching logic, extensive documentation, examples, and tests. The implementation is thorough, but I've identified a critical recursion bug in the wrapper's initialization. My review also includes feedback on a flawed test setup that fails to catch this bug, potential improvements for documentation maintainability, and enhancements to the provided example and test scripts.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…tions - Fix neptune-scale version check using importlib.metadata instead of __version__ - Run ruff format to fix code formatting issues - Add type annotations to NeptuneRunWrapper class attributes - Add type annotation for items list in _flatten_dict method
- Add image logging to test_real_neptune_without_mlop - Add image logging to test_real_neptune_with_mlop_dual_logging - Create test images using numpy and PIL - Log images using Neptune File objects via assign_files() and log_files() - Validates that image dual-logging works in production CI runs
- Add wait_for_submission() before close() to ensure files are uploaded - Change debug logging to info/warning for file conversion - Log successful file conversions and upload counts - Neptune Scale is async, files weren't uploading before close() was called
…ality - Neptune-only test: logs 3 grayscale images at steps 0, 1, 2 - Dual-logging test: logs 3 colored images (red/green/blue) at steps 0, 1, 2 - Also includes 1 static image via assign_files() - Images are 64x64 for better visibility
- Remove `|| echo` fallback patterns from CI workflow that were masking test failures - Change wait_for_submission() to wait_for_processing(verbose=False) to prevent "I/O operation on closed file" errors when pytest captures stdout/stderr - Update wrapper's __exit__ method to call wait_for_processing(verbose=False) before Neptune's __exit__ - Fix line-too-long errors in logging statements 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The TestNeptuneRealBackend tests access run._mlop_run which is only available when the monkeypatch is applied. Added 'import mlop.compat.neptune' before importing Run from neptune_scale to ensure the monkeypatch is applied first. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Neptune's close() method internally calls wait_for_processing() which logs status messages even when we call it with verbose=False before. When pytest captures stdout/stderr, these streams are closed before Neptune finishes logging, causing "I/O operation on closed file" errors. This fixture disables Neptune's logger during tests to prevent these errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Disable multiple Neptune-related loggers and remove their handlers to fully prevent I/O errors when pytest captures stdout/stderr. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The -s flag disables pytest's stdout/stderr capture. This prevents "I/O operation on closed file" errors that occur when Neptune's logger tries to write after pytest has closed the captured streams. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Configure logging.NullHandler and disable neptune_scale loggers before importing neptune_scale to prevent any I/O errors from Neptune's async operations trying to log after streams are closed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
asaiacai
force-pushed
the
neptune-compat
branch
from
04918b8 to
96eaeb1
Compare
When Python's logging handlers fail (e.g., writing to closed streams), the logging module normally prints error messages to stderr. Setting raiseExceptions to False suppresses these messages, preventing test failures caused by Neptune's async operations logging after pytest closes stdout/stderr. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Set logging.raiseExceptions before any other imports to ensure Python's logging module doesn't print error messages when handlers fail, regardless of what other modules might configure. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use int16 for intermediate calculations when adding noise to images, then clip and convert to uint8. This avoids UFuncOutputCastingError when adding int16 noise to uint8 arrays. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The mlop CLI is installed via poetry and needs to be invoked with 'poetry run mlop' to be found in the PATH. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Change from 'mlop:__main__.main' to 'mlop.__main__:main' to properly reference the main function in mlop/__main__.py. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add E402 per-file ignore for test_neptune_compat.py (logging must be configured before importing neptune_scale to prevent I/O errors) - Fix import sorting in neptune_migration_example.py - Apply ruff format to test file 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive compatibility layer for migrating from Neptune to mlop via dual-logging. The changes include the core monkeypatching logic, extensive documentation, example scripts, and a robust test suite. The implementation is well-designed with a strong focus on safety and ensuring existing Neptune functionality is not disrupted. I've identified a critical issue in an example script that would prevent the feature from working as described, and a medium-severity issue in the wrapper logic related to preserving step information. Once these points are addressed, this will be an excellent addition.
Address 4 outstanding issues from Gemini Code Assist review:
1. CRITICAL: Fix import order in examples
- Move mlop.compat.neptune import before Neptune imports
- Add I001 noqa to prevent ruff from reordering
- Affects: migrated_dual_logging_script, image_logging_example,
histogram_logging_example
- Impact: Enables dual-logging functionality (was broken before)
2. MEDIUM: Add missing step parameter in log_histograms
- Pass step parameter to mlop_run.log() for histograms
- Makes histogram logging consistent with log_files
- Location: mlop/compat/neptune.py:422
3. MEDIUM: Fix placeholder image paths in examples
- Replace 'path/to/image.png' with dynamically created temp images
- Use tempfile and PIL to create runnable examples
- Now example can be run without FileNotFoundError
4. MEDIUM: Consolidate documentation
- Reduce examples/neptune_migration_README.md from 344 to ~107 lines
- Link to main NEPTUNE_MIGRATION.md for comprehensive docs
- Prevents documentation drift between files
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Update documentation validation checks to match new README structure: - Replace "Configuration Options" check with "Example Files" - Replace "Supported Neptune Features" with "What Gets Logged to Both Systems" The consolidated README links to main NEPTUNE_MIGRATION.md for detailed configuration, so these section names changed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Allow users to disable Neptune API calls after Neptune sunset while keeping mlop logging active. This prevents errors from failed Neptune requests in post-sunset scenarios. Features: - Set DISABLE_NEPTUNE_LOGGING=true to skip all Neptune calls - All Neptune API methods become no-ops (no errors thrown) - mlop logging continues normally (mlop-only mode) - get_run_url() returns "neptune://disabled" placeholder - One INFO log at startup when Neptune is disabled Implementation: - Add _neptune_disabled flag checked in all wrapper methods - Skip Neptune initialization when flag is set - Update all 14 wrapper methods to check the flag - Return sensible defaults (None or placeholders) Documentation: - Add "Disabling Neptune (Post-Sunset)" section to main guide - Add example and usage table - Update examples README with post-sunset section Tests: - Add test_disable_neptune_logging in TestNeptuneCompatFallbackBehavior - Verify Neptune is not initialized when flag is set - Verify mlop logging continues working - Verify placeholder URLs are returned This provides a graceful kill switch for the Neptune sunset transition. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Comprehensive audit of environment variables used in Neptune compatibility layer: Environment Variables Documentation: - Add detailed table with code locations for all 6 env vars - Add "How Environment Variables Work" section explaining each var - Document DISABLE_NEPTUNE_LOGGING in main env vars table (was missing) - Verify MLOP_API_KEY actually works (passes to settings['_auth']) - Add code references for verification Neptune Scale Version: - Document tested version: neptune-scale 0.30.0 - Add compatibility matrix showing Python 3.10/3.11/3.12 tested - Pin neptune-scale to ">=0.30.0" in pyproject.toml (was "*") Verified Environment Variables: ✅ MLOP_PROJECT - Required, master switch for dual-logging ✅ MLOP_API_KEY - Optional, falls back to keyring, verified working ✅ MLOP_URL_APP - Optional, for self-hosted instances ✅ MLOP_URL_API - Optional, for self-hosted instances ✅ MLOP_URL_INGEST - Optional, for self-hosted instances ✅ DISABLE_NEPTUNE_LOGGING - Optional, post-sunset kill switch All env vars verified to actually work in the codebase with code references provided for audit trail. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The command 'mlop auth status' does not exist. The mlop CLI only has: - mlop login <token> - mlop logout Fixed troubleshooting section to use valid commands: - Check MLOP_API_KEY environment variable - Check keyring for stored credentials using Python This provides actual working debug steps instead of a non-existent command. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Backend PR #15 provides two tags update endpoints: - HTTP API: POST /api/runs/tags/update (for API key auth) - tRPC: runs.updateTags (for session auth) This commit aligns the Python client to use the correct HTTP API endpoint instead of the tRPC endpoint. Changes: - mlop/sets.py: Update URL to /api/runs/tags/update (was /trpc/runs.updateTags) - mlop/api.py: Use numeric runId, remove projectName from payload - mlop/iface.py: Update docstring to reflect HTTP API usage - TAGS_IMPLEMENTATION.md: Document actual implementation and backend integration - test_tags_unit.py: Add unit tests for payload format verification The HTTP endpoint is simpler and more appropriate for SDK use: - No tRPC batch format wrapping needed - Uses numeric run IDs (no SQID encoding) - No projectName field required - Designed for API key authentication All changes tested and verified to match backend expectations. Related: https://github.com/Trainy-ai/server-private/pull/15 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The DISABLE_NEPTUNE_LOGGING environment variable was being set by test_disable_neptune_logging but not cleaned up, causing subsequent tests to fail when they expected Neptune to be enabled. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Update neptune-migration-examples validation to check for: - 'Usage' instead of 'Example Files' - 'Supported Features' instead of 'What Gets Logged to Both Systems' These sections were renamed during documentation consolidation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
**Problem**: log_metrics() ignored user's explicit step parameter when logging to mlop, causing step misalignment during dual-logging. **Impact**: - Charts in Neptune vs mlop showed different x-axes - Violated user intent (explicit step=100 became step=1) - Broke dual-logging visualization alignment **Changes**: 1. mlop/compat/neptune.py: - Pass step to mlop: `self._mlop_run.log(data, step=step)` - Updated docstring to reflect step pass-through behavior - Replaced misleading comment about "acceptable step misalignment" 2. tests/test_neptune_compat.py: - Fixed assertion to expect step parameter - Added test_log_metrics_passes_step_to_mlop() test case - Imported 'call' from unittest.mock 3. examples/neptune_migration_README.md: - Updated compatibility matrix (step parameter passed through) - Added "Step Parameter Handling" documentation section - Explained behavior and provided examples **Testing**: - ✅ test_dual_logging_metrics_with_env_config - validates step=0 passed - ✅ test_log_metrics_passes_step_to_mlop - validates step=0, 100, 200 - ✅ All existing Neptune compat tests still pass **Consistency**: - log_files() already passed step correctly (line 409) - log_histograms() already passed step correctly (line 449) - Now log_metrics() matches this pattern 🤖 Generated with Claude Code (https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive compatibility layer to facilitate migration from Neptune to mlop by enabling dual-logging. The changes include monkeypatching the Neptune Run class, adding native tag support, and providing extensive documentation, examples, and tests. The implementation is robust, with a strong focus on ensuring that existing Neptune workflows are not disrupted.
My review identified two critical bugs in the NeptuneRunWrapper context manager implementation that occur when Neptune logging is disabled. I have also found several inconsistencies in the documentation regarding step parameter handling and the API used for tag updates, which could lead to user confusion. Additionally, there are minor issues with dependency management in pyproject.toml and some code duplication in the tests. Addressing these points will significantly improve the quality and reliability of this new feature.
Resolved critical and medium priority issues from Gemini code review: **Critical Fixes:** - Fix __enter__ method to handle DISABLE_NEPTUNE_LOGGING (prevents AttributeError) - Fix __exit__ method to handle DISABLE_NEPTUNE_LOGGING (prevents AttributeError) **Medium Priority Fixes:** - Update CLAUDE.md documentation to correct tags API endpoint (HTTP API, not tRPC) - Remove redundant suppress_neptune_logging fixture (module-level suppression already handles this) - Move numpy from dev to main dependencies (imported at module level in util.py, file.py, data.py) - Remove duplicate pillow entry from dev dependencies **Testing:** - All Neptune compatibility tests passing - Format and linting checks passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Update poetry.lock to reflect numpy being moved from dev to main dependencies. This is required because numpy is imported at module level in util.py, file.py, and data.py. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace global logging.basicConfig() with mlop-specific logger configuration. **Problem:** - Previous docs recommended logging.basicConfig(level=logging.WARNING) - This configures the ROOT logger globally, affecting ALL loggers - Can interfere with user's own logging configuration - Not a Python logging best practice **Solution:** - Configure only mlop-specific loggers: 'mlop' and 'mlop.compat.neptune' - Add console handler setup for pre-init debugging - Add note explaining when this configuration is needed **Benefits:** - No global side effects - Doesn't interfere with user's logging - Follows Python logging best practices - Still works for debugging mlop init failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Fix bug where initial tags from mlop.init(tags=['foo']) were not synced to server (tags were set after run creation request) - Pass tags through OpInit to Op.__init__() so they're included in initial create-run request - Remove unused sqid_encode/sqid_decode functions and sqids dependency (_op_id_encoded was set but never read anywhere) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Renamed test_real_dual_logging_integration to test_real_mlop_with_mock_neptune since it uses mock_neptune_backend fixture (Neptune is mocked, only mlop is real). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add POST /api/runs/config/update endpoint support (backend PR #19) - Add url_update_config to settings - Add make_compat_update_config_v1() payload function - Add _update_config() method to ServerInterface - Add update_config() method to Op for direct mlop usage - Update Neptune compat log_configs() to sync config to server - Add tests for update_config() and server sync verification 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
add compat monkeypatch package for neptune.
Tested (run the relevant ones):
bash format.sh