Skip to content

fix(BA-1459): Broken list_presets API, and SDK #4541

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 29, 2025
Merged

fix(BA-1459): Broken list_presets API, and SDK #4541

merged 3 commits into from
May 29, 2025

Conversation

jopemachine
Copy link
Member

@jopemachine jopemachine commented May 29, 2025 •
edited
Loading

resolves #4524 (BA-1459)

Tested manually.

Example code

async def test():
    async with AsyncSession() as session:
        r = await session.Resource.list()
        print(r)

Result

{'presets': [{'id': 'a3e29539-1a34-4180-be3f-6ab8a1687455', 'name': 'cpu01-small', 'shared_memory': None, 'resource_slots': {'cpu': '1', 'mem': '1073741824'}}, {'id': '34a9c5c4-6d16-4e27-b3bc-154e527852d2', 'name': 'cpu02-medium', 'shared_memory': None, 'resource_slots': {'cpu': '4', 'mem': '8589934592'}}, {'id': '00259dd8-a88d-4a67-b02e-d34726de3906', 'name': 'cpu03-large', 'shared_memory': None, 'resource_slots': {'cpu': '8', 'mem': '17179869184'}}, {'id': 'cfe1d74a-2ec5-4271-981d-83801fe6d313', 'name': 'cuda01-small', 'shared_memory': None, 'resource_slots': {'cpu': '1', 'mem': '4294967296'}}, {'id': '367c60b6-2fb5-47b6-a01b-72475ccb17c0', 'name': 'cuda02-medium', 'shared_memory': None, 'resource_slots': {'cpu': '4', 'mem': '17179869184'}}, {'id': '476b4713-653e-46d1-a71b-4e25e7e4d015', 'name': 'cuda03-large', 'shared_memory': None, 'resource_slots': {'cpu': '8', 'mem': '34359738368'}}]}

Checklist: (if applicable)

  • Mention to the original issue

@github-actions github-actions bot added size:XS ~10 LoC comp:manager Related to Manager component labels May 29, 2025
@jopemachine jopemachine changed the title fix: Broken ListResourcePresetsAction fix(BA-1459): Broken ListResourcePresetsAction May 29, 2025
@jopemachine jopemachine changed the title fix(BA-1459): Broken ListResourcePresetsAction fix(BA-1459): Broken list_presets API, and SDK May 29, 2025
@jopemachine jopemachine marked this pull request as ready for review May 29, 2025 02:02
Copilot
Copilot AI reviewed May 29, 2025
View reviewed changes
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 ensures the legacy etcd resource slots are loaded before streaming presets and records the fix in the changelog.

  • Preload resource slots via get_resource_slots() in list_presets
  • Add changelog entry for the fix

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/ai/backend/manager/services/resource_preset/service.py Invoke get_resource_slots() prior to streaming presets
changes/4541.fix.md Add changelog entry for the list_presets API and SDK fix
Comments suppressed due to low confidence (1)

src/ai/backend/manager/services/resource_preset/service.py:182

  • No tests were added to cover the new get_resource_slots() call or verify the behavior of loading presets; consider adding unit or integration tests for this path.
async def list_presets(self, action: ListResourcePresetsAction) -> ListResourceP

changes/4541.fix.md Outdated
@@ -0,0 +1 @@
Fix broken `list_presets` API, and SDK
Copy link
Preview

Copilot AI May 29, 2025

Choose a reason for hiding this comment

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

[nitpick] Remove the comma before 'and SDK' for clearer grammar: Fix broken list_presets API and SDK.

Copilot uses AI. Check for mistakes.

jopemachine
jopemachine commented May 29, 2025
View reviewed changes
src/ai/backend/manager/services/resource_preset/service.py
@@ -182,6 +182,7 @@ async def list_presets(self, action: ListResourcePresetsAction) -> ListResourceP
)
query = query.where(query_condition)
presets = []
await self._config_provider.legacy_etcd_config_loader.get_resource_slots()
Copy link
Member Author

Choose a reason for hiding this comment

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

This function relies on side effects, making its behavior invisible from the outside.
(It is passed via contextvar.)
However, since this part was originally written this way in the existing code, I left it as is for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah… this kind of code shouldn’t exist.
Fundamentally, there should be no code that accesses and updates a contextvar from another file.

@HyeockJinKim HyeockJinKim added this to the 24.09 milestone May 29, 2025
@HyeockJinKim HyeockJinKim added this pull request to the merge queue May 29, 2025
Merged via the queue into main with commit 7c051ba May 29, 2025
26 checks passed
@HyeockJinKim HyeockJinKim deleted the fix/broken_list_preset_rest_api branch May 29, 2025 05:18
lablup-octodog pushed a commit that referenced this pull request May 29, 2025
@jopemachine
fix(BA-1459): Broken list_presets API, and SDK (#4541)
d83f8b1 
Backported-from: main (25.8)
Backported-to: 25.6
Backport-of: 4541
@lablup-octodog lablup-octodog mentioned this pull request May 29, 2025
Closed
@github-actions GitHub Actions
Copy link

Backport to 24.09 is failed. Please backport manually.

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

Copilot code review Copilot Copilot left review comments

@HyeockJinKim HyeockJinKim HyeockJinKim approved these changes

Assignees

@jopemachine jopemachine

Labels
comp:manager Related to Manager component size:XS ~10 LoC
Projects
None yet
Milestone
24.09
Development

Successfully merging this pull request may close these issues.

Resource.list server error
2 participants
@jopemachine @HyeockJinKim