Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Refactor the configure_workers_and_start.py script used internally by Complement. #16650

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

reivilibre
Copy link
Contributor

@reivilibre reivilibre commented Nov 16, 2023

Base: develop

This pull request is intended for commit-by-commit review.

Original commit schedule, with full messages:

  1. Remove obsolete "app" from worker templates

  2. Convert worker templates into dataclass

  3. Use a lambda for the worker name rather than search and replace later

  4. Collapse WORKERS_CONFIG by removing entries with defaults

  5. Convert listener_resources and endpoint_patterns to Set[str]

  6. Tweak comments

  7. Add merge_into

  8. Remove special logic for adding stream_writers: just make it part of the extra config template

  9. Rename function to add_worker_to_instance_map given reduction of scope

  10. Add sharding_allowed to the WorkerTemplate rather than having a separate function for that

  11. Use merge_into when adding workers to the shared config

  12. Promote mark_filepath to constant

  13. Add a --generate-only option

@reivilibre reivilibre marked this pull request as ready for review November 17, 2023 12:48
@reivilibre reivilibre requested a review from a team as a code owner November 17, 2023 12:48
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Overall it seems like good changes, but I have a couple of questions.

docker/configure_workers_and_start.py Outdated Show resolved Hide resolved
docker/configure_workers_and_start.py Outdated Show resolved Hide resolved
docker/configure_workers_and_start.py Outdated Show resolved Hide resolved
docker/configure_workers_and_start.py Outdated Show resolved Hide resolved
docker/configure_workers_and_start.py Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants