[core] Feature/env vars settings#5
Conversation
This commit adds two new features to improve debugging and configuration:
1. MLOP_DEBUG_LEVEL environment variable
- Set logging level via environment (DEBUG, INFO, WARNING, ERROR, CRITICAL)
- Supports case-insensitive string values and numeric values
- Configuration precedence: function params > env var > default
2. Failed request tracking
- Logs failed HTTP requests to .mlop/{project}/{run}/failed_requests.log
- Only active when x_log_level <= DEBUG (no overhead otherwise)
- Thread-safe NDJSON format with timestamp, request type, URL, error details
- Captures failures after all retries exhausted
Changes:
- mlop/sets.py: Add MLOP_DEBUG_LEVEL environment variable reading in setup()
- mlop/iface.py: Add _log_failed_request() method and failure tracking in _try()
- tests/test_env_vars.py: Comprehensive test coverage (7 tests, all passing)
Tests: All 13 tests pass (7 skipped for optional dependencies)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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 significantly improves the configurability and debuggability of Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new failure logging mechanism in mlop/iface.py to record details of failed requests to a file, conditional on the debug log level. This includes adding _last_error_info and _lock_failure_log instance variables, a _log_failed_request method, and modifications to the _try method to capture and log error information upon retry exhaustion or unsuccessful responses. Concurrently, mlop/sets.py is updated to allow configuring the debug log level via the MLOP_DEBUG_LEVEL environment variable, with new tests in tests/test_env_vars.py validating this environment variable parsing. Review comments primarily raise concerns about a race condition in mlop/iface.py where the shared _last_error_info attribute could be overwritten by concurrent threads, leading to incorrect error logging, and suggest moving json and datetime imports to the top of the file.
I am having trouble creating individual review comments. Click here to see my feedback.
mlop/iface.py (100-101)
The _last_error_info attribute is a class member, which means it's shared across all instances of ServerInterface and, more importantly, across all threads that might be calling _try concurrently. This can lead to a race condition where one thread's error information overwrites another's before it can be logged, resulting in incorrect error reporting. Consider making _last_error_info local to the _try method or passing it explicitly through the call stack, or using a thread-local storage mechanism if the error needs to persist across retries within a single logical operation.
mlop/iface.py (409)
This line uses self._last_error_info which is a class member. As noted previously, this introduces a race condition. If multiple threads are making requests, the _last_error_info could be overwritten by another thread's error before this _log_failed_request call is made, leading to incorrect error information being logged. The error information should be local to the current request attempt or passed explicitly.
mlop/iface.py (421)
Storing r.text[:100] in _last_error_info is subject to the same race condition as mentioned earlier. If another thread encounters an error and updates _last_error_info before this thread's _log_failed_request is called, the wrong error message will be logged. Each _try invocation needs its own mechanism to store and retrieve error details.
mlop/iface.py (440)
Similar to the HTTP error case, assigning to self._last_error_info here creates a race condition. The exception e is specific to the current _try call, but storing it in a shared class member means it can be overwritten by concurrent operations. This could lead to _log_failed_request logging an error that didn't pertain to the specific failed request it's trying to log.
mlop/iface.py (370-371)
It's generally a best practice to place all import statements at the top of the file, outside of functions. Importing json and datetime inside _log_failed_request means they are re-imported every time the function is called, which can introduce a minor performance overhead and is less conventional. Moving these to the top of mlop/iface.py would improve maintainability and consistency.
import json
from datetime import datetime… env vars
This commit adds environment variable configuration for logging and server URLs:
1. MLOP_DEBUG_LEVEL environment variable
- Set logging level via environment (DEBUG, INFO, WARNING, ERROR, CRITICAL)
- Supports case-insensitive string values and numeric values
- Configuration precedence: function params > env var > default
2. Failed request tracking
- Logs failed HTTP requests to .mlop/{project}/{run}/failed_requests.log
- Only active when x_log_level <= DEBUG (no overhead otherwise)
- Thread-safe NDJSON format with timestamp, request type, URL, error details
- Captures failures after all retries exhausted
- Fixed race condition by passing error_info as parameter through recursion
3. URL configuration environment variables
- MLOP_URL_APP, MLOP_URL_API, MLOP_URL_INGEST, MLOP_URL_PY
- Allows pointing to custom server endpoints
- Configuration precedence: function params > env vars > defaults
Changes:
- mlop/sets.py: Add MLOP_DEBUG_LEVEL and URL environment variable reading in setup()
- mlop/iface.py:
- Add _log_failed_request() method and failure tracking in _try()
- Fix race condition in error tracking (per Gemini review)
- Move imports to top of file (per Gemini review)
- Fix deprecated datetime.utcnow() to use timezone.utc
- tests/test_env_vars.py: Comprehensive test coverage (14 tests, all passing)
Tests: All tests pass (14 env var tests + existing tests)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
4079ebf to
2235bb1
Compare
|
Summary This PR adds comprehensive environment variable support for configuring mlop at runtime, including logging levels, failed request tracking, and custom server URLs. Features
Configure logging verbosity via environment variable without code changes. Supported values (case-insensitive):
Usage: Configuration precedence: Function params > env var > default (16)
Automatically logs failed HTTP requests for debugging connectivity issues. Features:
Example log entry:
Point mlop to custom or self-hosted server endpoints. Environment variables:
Usage: Configuration precedence: Function params > env vars > defaults |
|
TODO: add documentation around settings and environment variable configuration |
adds env vars for configuration of
mlopat runtimeTested (run the relevant ones):
bash format.sh