Feat/phase4 fe postgres - #19
Merged
Merged
Conversation
…ndling, and CI/CD processes
…ublisher mode configuration
…schema initialization and error handling
…ate vector publishing
…ssion and error handling
📝 WalkthroughWalkthroughThe feature-engineering service now validates publisher settings, initializes a PostgreSQL publisher, emits state vectors in the background, exposes PostgreSQL health status, and provides Kubernetes deployment resources with integration and options-validation tests. ChangesFeature-engineering publishing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant FeatureEngineeringService
participant StateVectorEmitterService
participant PostgresStateWriter
participant PostgreSQL
FeatureEngineeringService->>PostgresStateWriter: EnsureReadyAsync
StateVectorEmitterService->>PostgresStateWriter: PublishAsync(StateVector)
PostgresStateWriter->>PostgreSQL: Insert vector bytes and metadata
PostgreSQL-->>PostgresStateWriter: Persisted result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
tests/AFIE.FeatureEngineering.Tests/Publishers/PostgresStateWriterTests.cs (1)
58-66: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify the stored vector bytes, not only the byte length.
This test passes if all 188 bytes have the wrong byte order or wrong values. Read
vectorasbyte[]and assert the expected little-endian float32 representation for the sample values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/AFIE.FeatureEngineering.Tests/Publishers/PostgresStateWriterTests.cs` around lines 58 - 66, Update PublishAsync_RoundTrip_VectorIs188Bytes to read the stored vector column as byte[] rather than only querying octet_length, then assert it matches the expected little-endian float32 byte representation produced by SampleVector().docs/development.md (1)
57-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant User Secrets initialization step.
AFIE.FeatureEngineering.csprojalready defines<UserSecretsId>at Line 6. Keep thedotnet user-secrets setcommand, or make initialization conditional for older checkouts. Official .NET guidance usesinitonly when the project has not already been initialized. (learn.microsoft.com)Proposed documentation change
-dotnet user-secrets init --project src/api/feature-engineering dotnet user-secrets set "FeatureEngineering:PostgresConnectionString" \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/development.md` around lines 57 - 61, Remove the redundant `dotnet user-secrets init` command from the documented setup steps, since `AFIE.FeatureEngineering.csproj` already defines `UserSecretsId`; retain the `dotnet user-secrets set` command unchanged..gitignore (1)
232-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
.terraform.lock.hcltracked.Line 236 ignores Terraform’s dependency lock file. Unlike
.terraform/and*.tfstate, this file records provider selections and checksums. Terraform guidance recommends reviewing and committing it with the root configuration. (developer.hashicorp.com)Proposed diff
- .terraform.lock.hcl🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore around lines 232 - 241, Remove the .terraform.lock.hcl entry from the Terraform ignore rules in .gitignore, while keeping the other generated Terraform artifacts ignored..claude/rules/CLAUDE.md (1)
47-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the health-check rule and implementation consistent.
FeatureEngineeringHealthCheckstill calculates staleness fromTelemetryScrapeIntervalSecondsandStalenessMultiplierconstants. It does not read these thresholds fromIOptions<T>. Add validated threshold options and rejection tests, or change this rule to document that the constants are intentional.As per coding guidelines, thresholds must come from
IOptions<T>and must not use magic constants.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/rules/CLAUDE.md around lines 47 - 54, Update the health-check rule and FeatureEngineeringHealthCheck implementation to obtain staleness thresholds through validated IOptions<T> settings instead of TelemetryScrapeIntervalSeconds and StalenessMultiplier constants. Add rejection tests covering invalid threshold values, and ensure the documented contract reflects the options-backed behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/feature-engineering/appsettings.json`:
- Around line 17-18: Validate FeatureEngineeringOptions.EmitIntervalSeconds in
its constructor so zero and negative values are rejected before
StateVectorEmitterService starts; ensure hosted-service startup cannot proceed
with an invalid interval, and add tests covering both 0 and negative values.
- Around line 17-18: Add the production deployment secret and Argo-syncable
manifest or deployment configuration for
FeatureEngineering__PostgresConnectionString, ensuring it is available before
service startup when PublisherMode is postgres. Use the existing production
PostgreSQL secret/configuration conventions and preserve the current appsettings
defaults.
In `@src/api/feature-engineering/Models/FeatureEngineeringOptions.cs`:
- Line 18: Enforce that FeatureEngineeringOptions.EmitIntervalSeconds is
strictly positive during options binding/validation, rejecting zero and negative
values before the host starts. Add the appropriate options validation metadata
or validator to FeatureEngineeringOptions and ensure it is registered for
startup validation; StateVectorEmitterService requires no direct change because
it should continue consuming the validated value at its existing Task.Delay
call.
In `@src/api/feature-engineering/Program.cs`:
- Around line 44-48: Update src/api/feature-engineering/Program.cs lines 44-48
to explicitly accept only supported PublisherMode values, failing startup for
azureml until its persistence is implemented and for unknown values instead of
defaulting to Postgres. Update
src/api/feature-engineering/Publishers/AzureMlFeatureStorePublisher.cs lines
11-17 so its readiness and publishing methods do not report success or complete
successfully without actual Azure ML persistence.
In `@src/api/feature-engineering/Publishers/PostgresStateWriter.cs`:
- Around line 76-80: Update the exception handling in PublishAsync around the
Postgres INSERT: keep setting _health.PostgresReachable to false and logging the
error, rethrow cancellation exceptions unchanged to preserve cancellation, and
rethrow other failures so StateVectorEmitterService records the workload as
failed instead of counting it as emitted.
- Around line 60-61: The vector serialization in PostgresStateWriter must
produce pgvector BYTEA format rather than platform-native bytes: update the
conversion before binding `@Vector` to encode float32 values in PostgreSQL network
byte order and include the required 4-byte header. In
tests/AFIE.FeatureEngineering.Tests/Publishers/PostgresStateWriterTests.cs lines
58-66, extend PublishAsync_RoundTrip_VectorIs188Bytes to assert both the stored
length and exact byte content/order for known float values.
---
Nitpick comments:
In @.claude/rules/CLAUDE.md:
- Around line 47-54: Update the health-check rule and
FeatureEngineeringHealthCheck implementation to obtain staleness thresholds
through validated IOptions<T> settings instead of TelemetryScrapeIntervalSeconds
and StalenessMultiplier constants. Add rejection tests covering invalid
threshold values, and ensure the documented contract reflects the options-backed
behavior.
In @.gitignore:
- Around line 232-241: Remove the .terraform.lock.hcl entry from the Terraform
ignore rules in .gitignore, while keeping the other generated Terraform
artifacts ignored.
In `@docs/development.md`:
- Around line 57-61: Remove the redundant `dotnet user-secrets init` command
from the documented setup steps, since `AFIE.FeatureEngineering.csproj` already
defines `UserSecretsId`; retain the `dotnet user-secrets set` command unchanged.
In `@tests/AFIE.FeatureEngineering.Tests/Publishers/PostgresStateWriterTests.cs`:
- Around line 58-66: Update PublishAsync_RoundTrip_VectorIs188Bytes to read the
stored vector column as byte[] rather than only querying octet_length, then
assert it matches the expected little-endian float32 byte representation
produced by SampleVector().
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86132008-b881-48cf-a601-d2cf428d86b8
📒 Files selected for processing (15)
.claude/rules/CLAUDE.md.gitignoredocs/development.mdsrc/api/feature-engineering/AFIE.FeatureEngineering.csprojsrc/api/feature-engineering/Health/FeatureEngineeringHealthCheck.cssrc/api/feature-engineering/Models/FeatureEngineeringOptions.cssrc/api/feature-engineering/Program.cssrc/api/feature-engineering/Publishers/AzureMlFeatureStorePublisher.cssrc/api/feature-engineering/Publishers/IStateVectorPublisher.cssrc/api/feature-engineering/Publishers/PostgresStateWriter.cssrc/api/feature-engineering/Services/StateVectorEmitterService.cssrc/api/feature-engineering/appsettings.jsonsrc/api/feature-engineering/experiments/state/fe_consumer_offset.jsontests/AFIE.FeatureEngineering.Tests/AFIE.FeatureEngineering.Tests.csprojtests/AFIE.FeatureEngineering.Tests/Publishers/PostgresStateWriterTests.cs
💤 Files with no reviewable changes (1)
- src/api/feature-engineering/experiments/state/fe_consumer_offset.json
…rvice constructor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/feature-engineering/Services/StateVectorEmitterService.cs (1)
57-68: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate PostgreSQL write failures.
PostgresStateWriter.PublishAsynccatches insert exceptions and returns normally. Line 59 then incrementsemitted, and Line 68 logs a successful emission for a vector that PostgreSQL did not store.Update
PostgresStateWriter.PublishAsyncto update health state, log the failure, and rethrow. Also rethrowOperationCanceledExceptionso shutdown can stop pending emissions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/feature-engineering/Services/StateVectorEmitterService.cs` around lines 57 - 68, Update PostgresStateWriter.PublishAsync so PostgreSQL insert failures update the health state, log the exception, and rethrow instead of returning normally; also propagate OperationCanceledException unchanged so cancellation reaches the emission loop. Keep StateVectorEmitterService’s emitted count and success logging dependent on PublishAsync completing successfully.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@infra/gitops/manifests/feature-engineering-deployment.yaml`:
- Around line 66-71: Replace the emptyDir backing the feature-engineering-data
volume in the feature-engineering deployment with the shared persistent storage
used by the result producer, ensuring it is mounted at /app/experiments and
retains both producer results and consumer offset state across pod restarts.
- Around line 47-58: Update the livenessProbe configuration to use the
process-only health endpoint instead of the PostgreSQL-gated /health path, while
preserving /health for readinessProbe and any startup probe configuration.
- Around line 17-68: Harden the feature-engineering Deployment by configuring
pod and container securityContext values for non-root execution, disabled
privilege escalation, dropped Linux capabilities, and a read-only root
filesystem. Update the image used by the feature-engineering container to run as
a non-root user, and add explicit writable emptyDir volumes and mounts for the
required /app/experiments paths while leaving the rest of the filesystem
read-only.
In `@infra/gitops/manifests/feature-engineering-secret.example.yaml`:
- Around line 9-11: Exclude the feature-engineering Secret template from Argo
reconciliation by renaming feature-engineering-secret.example.yaml with a
non-manifest extension, moving it outside the recursively synced manifests
directory, or adding an explicit exclude in app-afie.yaml. Preserve the template
for reference while ensuring Argo cannot apply its REPLACE_ME values.
---
Outside diff comments:
In `@src/api/feature-engineering/Services/StateVectorEmitterService.cs`:
- Around line 57-68: Update PostgresStateWriter.PublishAsync so PostgreSQL
insert failures update the health state, log the exception, and rethrow instead
of returning normally; also propagate OperationCanceledException unchanged so
cancellation reaches the emission loop. Keep StateVectorEmitterService’s emitted
count and success logging dependent on PublishAsync completing successfully.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ecadacd3-6c56-42c8-8b74-f08b29ed907b
📒 Files selected for processing (4)
infra/gitops/manifests/feature-engineering-deployment.yamlinfra/gitops/manifests/feature-engineering-secret.example.yamlsrc/api/feature-engineering/Services/StateVectorEmitterService.cstests/AFIE.FeatureEngineering.Tests/Services/StateVectorEmitterServiceTests.cs
…ption for methods
…ty on Argo sync behavior
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
infra/gitops/manifests/feature-engineering-secret.example (1)
9-11: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an explicit Argo CD skip directive.
targeetRevision: mainrecurses underinfra/gitops/manifests/. A future rename to.yamlwould let Argo CD render this placeholder Secret and overwritefeature-engineering-secretswithREPLACE_MEvalues. Add# +argocd:skip-file-renderingso the file remains a template only, and keep the.exampleextension as a secondary safeguard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/gitops/manifests/feature-engineering-secret.example` around lines 9 - 11, Add the explicit # +argocd:skip-file-rendering directive to the feature-engineering secret template, while preserving its .example extension and existing explanatory comments.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/feature-engineering/Publishers/PostgresStateWriter.cs`:
- Around line 80-84: Validate vector.Values serializes to exactly 188 bytes
before the PostgreSQL insert in the surrounding write method, and reject invalid
payloads without entering the generic database exception handler. Keep
_health.PostgresReachable unchanged for this validation failure, while
preserving existing outage handling for actual PostgreSQL errors, and add
coverage for an invalid vector.
---
Nitpick comments:
In `@infra/gitops/manifests/feature-engineering-secret.example`:
- Around line 9-11: Add the explicit # +argocd:skip-file-rendering directive to
the feature-engineering secret template, while preserving its .example extension
and existing explanatory comments.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a091b872-cc6b-45c7-9e7c-6f98cdf8cdc0
📒 Files selected for processing (9)
infra/gitops/manifests/feature-engineering-deployment.yamlinfra/gitops/manifests/feature-engineering-secret.examplesrc/api/feature-engineering/Models/FeatureEngineeringOptions.cssrc/api/feature-engineering/Program.cssrc/api/feature-engineering/Publishers/AzureMlFeatureStorePublisher.cssrc/api/feature-engineering/Publishers/PostgresStateWriter.cssrc/api/feature-engineering/Services/StateVectorEmitterService.cstests/AFIE.FeatureEngineering.Tests/Models/FeatureEngineeringOptionsValidationTests.cstests/AFIE.FeatureEngineering.Tests/Publishers/PostgresStateWriterTests.cs
💤 Files with no reviewable changes (1)
- src/api/feature-engineering/Services/StateVectorEmitterService.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- infra/gitops/manifests/feature-engineering-deployment.yaml
- tests/AFIE.FeatureEngineering.Tests/Publishers/PostgresStateWriterTests.cs
- src/api/feature-engineering/Models/FeatureEngineeringOptions.cs
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.
Summary
IStateVectorPublisherstrategy:PostgresStateWriter(Dapper + Npgsql, BYTEA blob, schema init on boot) as the active
implementation,
AzureMlFeatureStorePublisheras a Phase-8 stub.StateVectorEmitterService— 60s BackgroundService that iteratesWindowStore.Workloadsand persists a vector per workload per tick./health: the standardAddNpgSqlcheck plus gating
FeatureEngineeringHealthCheckon!PostgresReachable.appsettings.jsonomitsPostgresConnectionString; dev sourcing isdotnet user-secrets,prod sourcing (PR 6) is a Kubernetes Secret via env var.
Program.csfails fast at boot with a message pointing developers to the right
setup command.
docs/development.mdgains a "Local Postgres for Feature Engineering"section documenting the one-time User Secrets setup.
AspNetCore.HealthChecks.NpgSql 8.0.1 (runtime);
Testcontainers.PostgreSql 3.10.0 (tests only).
Design notes
broken emitter loop. The K8s init container in PR 6 gates FE on
pg_isreadyso this is safe in-cluster.NpgsqlDataSourcesingleton uses built-in pooling; no per-writeconnection creation.
CHECK; matches how the Phase 5 Python trainer will
np.frombuffer.runtime (User Secrets in dev, Kubernetes Secret in prod).
Test plan
dotnet build AFIE.slnx— clean (one pre-existing xUnit2013warning in the PR 3 test, unrelated)
dotnet test AFIE.slnx— 63 passing (20 telemetry + 43 FE,requires Docker for Testcontainers)
exits with the "not configured" message
docker run postgres:16-alpine: append aMetricEvent to today's JSONL; after 60s,
state_vectorshas ≥1row with
octet_length(vector)=188/healthshows bothfeature-engineeringandpostgresHealthy;stateVectorsWrittenTotal >= 1CI note
Testcontainers requires Docker on the runner. Ensure
docker-in-dockeravailability or the equivalent GitHub Actions service. CI needs no
secret injection for the tests themselves — Testcontainers spins up its
own ephemeral Postgres per test class.
Summary by CodeRabbit
New Features
Bug Fixes
Tests