feat(flags): Implement local evaluation for flag dependencies#288
feat(flags): Implement local evaluation for flag dependencies#288
Conversation
eaa5ce5 to
3fa550d
Compare
61fe1fd to
4bccee9
Compare
6e85b32 to
15c6f29
Compare
15c6f29 to
94d482e
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request implements local evaluation support for feature flag dependencies, allowing flags to depend on the evaluation results of other flags during local evaluation. The implementation includes a comprehensive dependency graph system with cycle detection, proper evaluation ordering, and robust error handling.
Key changes:
- Added a complete dependency graph system that builds relationships between flags and evaluates them in topological order
- Enhanced the client to use dependency-aware evaluation when flag dependencies are detected
- Updated the feature flag evaluation logic to support flag property matching using cached dependency results
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| posthog/dependency_graph.py | New module implementing dependency graph data structure, topological sorting, and flag dependency evaluation logic |
| posthog/client.py | Enhanced client to build dependency graphs on flag assignment and use dependency-aware evaluation |
| posthog/feature_flags.py | Updated flag evaluation functions to accept and use dependency graph parameters for flag property matching |
| posthog/test/test_dependency_graph.py | Comprehensive unit tests for dependency graph functionality |
| posthog/test/test_client_flag_dependencies.py | Integration tests for client-level flag dependency features |
| posthog/test/test_feature_flags.py | Updated existing tests to work with new flag dependency format |
| posthog/test/manual_integration/ | Added manual integration test infrastructure and flag dependency integration tests |
| pyproject.toml | Excluded manual integration tests from automatic test runs |
Comments suppressed due to low confidence (1)
posthog/feature_flags.py:145
- [nitpick] The warning message format is inconsistent with other similar warnings in the codebase. Consider using a format similar to line 387 for consistency.
log.warning(
There was a problem hiding this comment.
Greptile Summary
This PR implements local evaluation support for feature flag dependencies in the PostHog Python SDK. The core change enables feature flags to have conditions based on how other flags are evaluated, avoiding the need to fall back to remote evaluation for dependency scenarios.
The implementation introduces a new dependency_graph.py module that provides:
- A
DependencyGraphclass for managing flag relationships with cycle detection and topological sorting - Graph algorithms including Kahn's algorithm for dependency ordering and DFS for cycle detection
- Functions like
build_dependency_graph(),evaluate_flags_with_dependencies(), andmatch_flag_property()for orchestrating dependency-aware evaluation
In client.py, the changes integrate dependency support by:
- Adding
dependency_graphandid_to_key_mappinginstance variables to track flag relationships - Building dependency graphs automatically when feature flags are loaded via the
feature_flagssetter - Modifying
_locally_evaluate_flag()to use dependency-aware evaluation when dependencies exist - Threading dependency context through all flag evaluation functions
The feature_flags.py module is updated to replace previous warning-only behavior for flag dependencies with actual evaluation using the new dependency system. The core evaluation functions now accept dependency_graph and id_to_key parameters to resolve flag dependencies at any level of the evaluation hierarchy.
The implementation includes comprehensive error handling, graceful fallbacks when dependency evaluation fails, caching of evaluation results to avoid re-computation, and extensive test coverage including unit tests, integration tests, and manual integration tests for end-to-end validation.
Confidence score: 3/5
- This implementation is functionally sound but contains several security and structural concerns that need attention before merging.
- The score reflects hardcoded API keys in test files, incorrect documentation paths, and some defensive programming gaps, though the core dependency evaluation logic appears robust.
- Files needing attention:
posthog/test/manual_integration/test_flag_dependencies.py(security issues with hardcoded keys),posthog/client.py(error handling improvements), and documentation corrections in README files.
11 files reviewed, 6 comments
94d482e to
c950d84
Compare
Flags now support filter conditions that depend on how other flags were evaluated. This brings that support to local evaluation in posthog-python. This commit includes unit and integration tests.
c950d84 to
6dabe6e
Compare
When group_type_mapping is missing, the system should fall back to /decide/ endpoint rather than returning False. This matches the behavior of the original _compute_flag_locally method.
| @@ -0,0 +1,535 @@ | |||
| import logging | |||
There was a problem hiding this comment.
My main concern is adding dependency resolution logic to the SDKs will make them more complex and harder to maintain.
Is it possible to move the dependency resolution logic to the server? That way, the local evaluation endpoint returns flags that are already resolved and can be evaluated
There was a problem hiding this comment.
Oh interesting! If we return the flags in evaluation order, then the SDK logic gets way simpler. Great idea!
We can also transform the filters when sending them to the client so IDs are already resolved to the flag keys.
|
Going to submit a new PR based on the updated format for local_evaluation: PostHog/posthog#36693 |
Feature flags will soon be able to have conditions based on how other flags are evaluated. This PR adds support for that in local evaluation.
I'm going to create it as a DRAFT because there's some open questions about the flag dependency format in the referenced PR that need to be resolved before we back this in.