Skip to content

fix container group caching issues#531

Merged
ms-lolo merged 5 commits into
mainfrom
fix/container-group-caching
May 28, 2025
Merged

fix container group caching issues#531
ms-lolo merged 5 commits into
mainfrom
fix/container-group-caching

Conversation

@ms-lolo

@ms-lolo ms-lolo commented May 27, 2025

Copy link
Copy Markdown
Contributor

calling rats.apps.Container.get_group() a second time for a given service id was returning an empty list, because we're caching the group generator, which gets exhausted after the first time looping through results. fixed by caching the list instead of the generator. also added a unit test for this behavior.

@ms-lolo ms-lolo requested a review from Copilot May 27, 2025 18:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 fixes container group caching issues by converting service group generators into lists, ensuring that subsequent calls yield consistent results. It also adds a corresponding unit test and updates the app composition to include an additional groups plugin.

  • Fix caching issue by caching the list instead of an exhausted generator.
  • Add unit tests to validate the correct behavior.
  • Update container composition by using a composite container to include the groups plugin.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
rats-apps/test/rats_test/apps/test_service_caching.py Added unit tests for group caching behavior.
rats-apps/test/rats_test/apps/example/_app.py Updated container composition to include ExampleGroupsPlugin1.
rats-apps/src/rats/apps/_container.py Modified caching strategy by converting generators to lists for specific namespaces.

Comment thread rats-apps/src/rats/apps/_container.py
@ms-lolo ms-lolo requested a review from jzazo May 27, 2025 18:12
@codecov

codecov Bot commented May 27, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.90%. Comparing base (f03955e) to head (b5e87cb).
Report is 2 commits behind head on main.

Additional details and impacted files
Components Coverage Δ
rats-apps ∅ <ø> (∅)
rats-devtools ∅ <ø> (∅)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jzazo

jzazo commented May 28, 2025

Copy link
Copy Markdown
Collaborator

the problem with caching an iterator or a list, is that the group cannot later change? Should we cache the objects rather than the group itself?

Otherwise, I am happy to proceed with the PR if it unblocks you.

@jzazo jzazo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

See comment on caching the items rather than the group

@ms-lolo

ms-lolo commented May 28, 2025

Copy link
Copy Markdown
Contributor Author

the problem with caching an iterator or a list, is that the group cannot later change? Should we cache the objects rather than the group itself?

Otherwise, I am happy to proceed with the PR if it unblocks you.

not sure i understood, service providers are always immutable and never change, so our goal is to cache all the calls to get a service or group.

@jzazo

jzazo commented May 28, 2025

Copy link
Copy Markdown
Collaborator

I guess service providers are always immutable, that includes groups? If that's the premise, go ahead and cache the group as a list/tuple.

@ms-lolo ms-lolo merged commit c58c670 into main May 28, 2025
20 checks passed
@ms-lolo ms-lolo deleted the fix/container-group-caching branch May 28, 2025 18:52
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.

3 participants