Skip to content

Conversation

jcabrero
Copy link
Member

No description provided.

@jcabrero jcabrero requested a review from Copilot October 10, 2025 16:01
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR migrates the system from using etcd to Redis for model service discovery. The change affects the underlying storage mechanism for model registration and discovery while maintaining the same API surface.

  • Replaced etcd3gw client with redis.asyncio for all model discovery operations
  • Updated configuration to use Redis connection parameters instead of etcd parameters
  • Modified Docker Compose files to use Redis container dependencies instead of etcd

Reviewed Changes

Copilot reviewed 36 out of 36 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/nilai-common/src/nilai_common/discovery.py Core migration from etcd to Redis with async/await pattern
packages/nilai-common/pyproject.toml Updated dependency from etcd3gw to redis
tests/unit/nilai-common/test_discovery.py Comprehensive test rewrite using Redis test containers
tests/unit/nilai-common/conftest.py Added Redis test container fixtures
nilai-api/src/nilai_api/state.py Added async initialization for discovery service
nilai-api/src/nilai_api/config/* Updated configuration models from etcd to discovery
docker-compose*.yml Updated all compose files to depend on redis instead of etcd

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +30 to +33
discovery_host=str(os.getenv("ETCD_HOST", "localhost")),
discovery_port=int(
os.getenv("ETCD_PORT", 6379)
), # Redis port (changed from etcd's 2379)
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The environment variable names still reference ETCD_HOST and ETCD_PORT despite migrating to Redis. Consider renaming these to DISCOVERY_HOST and DISCOVERY_PORT for consistency and to avoid confusion.

Suggested change
discovery_host=str(os.getenv("ETCD_HOST", "localhost")),
discovery_port=int(
os.getenv("ETCD_PORT", 6379)
), # Redis port (changed from etcd's 2379)
discovery_host=str(os.getenv("DISCOVERY_HOST", "localhost")),
discovery_port=int(
os.getenv("DISCOVERY_PORT", 6379)
), # Redis port

Copilot uses AI. Check for mistakes.

Comment on lines +46 to +47
@property
async def client(self) -> redis.Redis:
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using @Property decorator with async method is unconventional and may cause confusion. Consider making this a regular async method like async def get_client(self) instead.

Copilot uses AI. Check for mistakes.

Comment on lines +53 to +55
if self._client is None:
# This should never happen
raise ValueError("Redis client must be initialized")
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The double check for self._client is None is redundant since initialize() should always set self._client. The second check could be replaced with an assertion or removed entirely.

Suggested change
if self._client is None:
# This should never happen
raise ValueError("Redis client must be initialized")
assert self._client is not None, "Redis client must be initialized after initialize()"

Copilot uses AI. Check for mistakes.

Comment on lines +47 to +48
ETCD_HOST: redis
ETCD_PORT: "6379"
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Environment variable names ETCD_HOST and ETCD_PORT are misleading after migrating to Redis. These should be updated to DISCOVERY_HOST and DISCOVERY_PORT to match the new architecture.

Suggested change
ETCD_HOST: redis
ETCD_PORT: "6379"
DISCOVERY_HOST: redis
DISCOVERY_PORT: "6379"

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant