Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refact: Make topic explicit in message queue API #358

Merged
merged 5 commits into from
Nov 13, 2024
Merged

Conversation

masci
Copy link
Member

@masci masci commented Nov 12, 2024

Apologies for the mega-refactoring, breaking this down into smaller PR would take a week 😛

Fixes #349 for good.

Problem

Message publishers and subcribers assumed the "topic" to listen to was the same as the message type. This works in a single-tenant environment but becomes a problem when the same message queue holds data for multiple control planes and services, causing clashes.

Solution

  • Make the concept of "topic" explicit, so that when publishing/reading messages a topic must be provided
  • Make the topics "nested" under the control plane, like control_plane_one.my_service so that services with the same name attached to different control planes can coexist
  • Make the topic "namespace" configurable, so that users can specify where to nest topics used in Llama Deploy and avoid conflicts with their existing, unrelated topics

Follow up

  • Fix topic management for the other message queues we support, for now only Kafka is taking advantage of this feature

@coveralls
Copy link

coveralls commented Nov 12, 2024

Coverage Status

coverage: 72.127% (+0.03%) from 72.1%
when pulling 1997f1e on massi/topic-refact
into 1fcfa2f on main.

e2e_tests/apiserver/test_streaming.py Show resolved Hide resolved
llama_deploy/client/models/apiserver.py Show resolved Hide resolved
llama_deploy/client/models/model.py Show resolved Hide resolved
llama_deploy/control_plane/server.py Show resolved Hide resolved
llama_deploy/message_queues/rabbitmq.py Show resolved Hide resolved
llama_deploy/message_queues/redis.py Show resolved Hide resolved
tests/client/models/test_apiserver.py Show resolved Hide resolved
tests/client/models/test_model.py Show resolved Hide resolved
tests/control_plane/test_control_plane.py Show resolved Hide resolved
Copy link
Collaborator

@logan-markewich logan-markewich left a comment

Choose a reason for hiding this comment

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

Hmm, this makes sense. Trying to understand the hierarchy of control now

  • task definitions have an agent_id specifying what service to send to (this probably needs to be renamed in the future)
  • messages have a type used for judging how a service handles a message
  • the topic for a consumer is defined by the control plane config topic + msg type?
    • does this mean each service + message type combination has its own topic/queue now?

Slightly fuzzy if there's anything beyond this 😅 I feel like I need to draw this out now

@masci
Copy link
Member Author

masci commented Nov 12, 2024

Hmm, this makes sense. Trying to understand the hierarchy of control now

* task definitions have an `agent_id` specifying what service to send to (this probably needs to be renamed in the future)

Correct, this didn't change.

* messages have a `type` used for judging how a service handles a message

Same as before

* the `topic` for a consumer is defined by the control plane config topic + msg type?

Correct, this is brand new

  * does this mean each service + message type combination has its own topic/queue now?

Exactly, which makes it possible to share the same Kafka cluster (but really any message queue broker) across multiple control planes

Slightly fuzzy if there's anything beyond this 😅 I feel like I need to draw this out now

That's really it :)

@logan-markewich
Copy link
Collaborator

@masci thanks for confirming!

Copy link
Contributor

@nerdai nerdai left a comment

Choose a reason for hiding this comment

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

LGTM -- wow, this turned out to be a big one.

Should we add some issues/tickets for making topic's explicit with the other existing message queue integrations?

# register to control plane
control_plane_url = (
f"http://{control_plane_config.host}:{control_plane_config.port}"
)
await service.register_to_control_plane(control_plane_url)

# register to message queue
consumer_fn = await service.register_to_message_queue()

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if its a pre-requisite to register to the service (and its topic) to the message queue.

They both need to happen, and both are viewed as pre-requisites ofc for a user to send a task to a service:

User sends task -> control plane publishes message with associated topic -> message queue pushes the message to the correct topic queue -> the service consumes the task


@property
def publish_callback(self) -> Optional[PublishCallback]:
return None

@abstractmethod
def get_topic(self, msg_type: str) -> str: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

I think before we were thinking of delegating such responsibilities to the control plane, which should then know about all of the existing topics.

@masci
Copy link
Member Author

masci commented Nov 13, 2024

Follow up issues:

@masci masci merged commit 4449d9f into main Nov 13, 2024
10 checks passed
@masci masci deleted the massi/topic-refact branch November 13, 2024 08:16
@masci masci added the enhancement New feature or request label Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There is a conflict between control planes which are reading from the same topic called "control_plane"
4 participants