[ENH] For chroma cloud efs, extract api key from header if available to authenticate #5914
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
jairad26
force-pushed
the
jai/cloud-ef-api-key-extraction
branch
2 times, most recently
from
994803a to
a92603d
Compare
|
Add fallback API-key extraction from existing Chroma Cloud clients Introduces a static helper Key Changes• Added Affected Areas• This summary was automatically generated by @propel-code-bot |
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
| The first api key found, or None if no client instances have api keys set. | ||
| """ | ||
| # Check FastAPI instances' session headers - this is where both paths converge | ||
| for system in SharedSystemClient._identifier_to_system.values(): |
There was a problem hiding this comment.
[Logic] Race condition: _identifier_to_system dictionary is accessed without synchronization. If multiple threads call get_chroma_cloud_api_key_from_clients() while another thread modifies the dictionary (via __init__ or clear_system_cache()), this can raise RuntimeError: dictionary changed size during iteration or return inconsistent results.
@staticmethod
def get_chroma_cloud_api_key_from_clients() -> Optional[str]:
# Create snapshot to avoid concurrent modification issues
systems_snapshot = list(SharedSystemClient._identifier_to_system.values())
for system in systems_snapshot:
# ... rest of logicAlternatively, protect dictionary access with a lock if thread-safety is required.
Context for Agents
Race condition: `_identifier_to_system` dictionary is accessed without synchronization. If multiple threads call `get_chroma_cloud_api_key_from_clients()` while another thread modifies the dictionary (via `__init__` or `clear_system_cache()`), this can raise `RuntimeError: dictionary changed size during iteration` or return inconsistent results.
```python
@staticmethod
def get_chroma_cloud_api_key_from_clients() -> Optional[str]:
# Create snapshot to avoid concurrent modification issues
systems_snapshot = list(SharedSystemClient._identifier_to_system.values())
for system in systems_snapshot:
# ... rest of logic
```
Alternatively, protect dictionary access with a lock if thread-safety is required.
File: chromadb/api/shared_system_client.py
Line: 108| except Exception: | ||
| # if we can't access the ServerAPI instance or it doesn't have _session, | ||
| # continue to the next system instance | ||
| continue |
There was a problem hiding this comment.
[Reliability] The broad except Exception silently catches all errors, including critical issues like AttributeError from API changes or KeyError from malformed data structures. This makes debugging difficult when the code fails.
except AttributeError as e:
# ServerAPI doesn't have expected attributes (_session or _api_url)
logger.debug(f"Skipping system {system_id}: {e}")
continue
except Exception as e:
# Unexpected errors should be logged for investigation
logger.warning(f"Unexpected error extracting API key from system: {e}")
continueThis distinguishes expected structural variations from genuine errors that need attention.
Context for Agents
The broad `except Exception` silently catches all errors, including critical issues like `AttributeError` from API changes or `KeyError` from malformed data structures. This makes debugging difficult when the code fails.
```python
except AttributeError as e:
# ServerAPI doesn't have expected attributes (_session or _api_url)
logger.debug(f"Skipping system {system_id}: {e}")
continue
except Exception as e:
# Unexpected errors should be logged for investigation
logger.warning(f"Unexpected error extracting API key from system: {e}")
continue
```
This distinguishes expected structural variations from genuine errors that need attention.
File: chromadb/api/shared_system_client.py
Line: 133
| # If not found in env var, try to get it from existing client instances | ||
| if not self.api_key: | ||
| raise ValueError(f"The {api_key_env_var} environment variable is not set.") | ||
| from chromadb.api.shared_system_client import SharedSystemClient |
There was a problem hiding this comment.
Please do not do inline imports. Why is this needed?
jairad26
force-pushed
the
jai/cloud-ef-api-key-extraction
branch
from
a92603d to
1e64695
Compare
jairad26
force-pushed
the
jai/cloud-ef-api-key-extraction
branch
from
1e64695 to
6abaca0
Compare
Description of changes
Summarize the changes made by this PR.
api.trychroma.comTest plan
How are these changes tested?
Added tests for extraction function
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?
Observability plan
What is the plan to instrument and monitor this change?
Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?