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.

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

feat: Replace sessions, kernels's status_history's type dict with list #3201

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

feat: Replace sessions, kernels's status_history's type dict with list #3201

wants to merge 12 commits into from

Conversation

jopemachine
Copy link
Member

@jopemachine jopemachine commented Dec 5, 2024 •
edited by github-actions bot
Loading

Resolves #3200, Part of #3214.
Refs #412, follow-up to #480.

The current implementation saves only the most recent timestamp whenever status information in status_history is updated, and all previous information is deleted.

This PR prevents the loss of timestamp information by changing the data structure of sessions, kernels's status_history to List.

Previous format:

{
  "PENDING": "2024-01-01T00:00:05Z",
  "SCHEDULED": "2024-01-01T00:00:10Z",
  "PREPARING": "2024-01-01T00:00:12Z",
  "RESTARTING": "2024-01-01T00:30:00Z",
  "RUNNING": "2024-01-01T00:30:08Z",  // overwritten if restarted
}

New format:

[
  {"status": "PENDING", "timestamp": "2024-01-01T00:00:05Z"},
  {"status": "SCHEDULED", "timestamp": "2024-01-01T00:00:05Z"},
  {"status": "PREPARING", "timestamp": "2024-01-01T00:00:05Z"},
  {"status": "RUNNING", "timestamp": "2024-01-01T00:00:15Z"},  // preserved
  {"status": "RESTARTING", "timestamp": "2024-01-01T00:30:00Z"},
  {"status": "RUNNING", "timestamp": "2024-01-01T00:30:08Z"},
]

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • API server-client counterparts (e.g., manager API -> client SDK)

📚 Documentation preview 📚: https://sorna--3201.org.readthedocs.build/en/3201/

📚 Documentation preview 📚: https://sorna-ko--3201.org.readthedocs.build/ko/3201/

@github-actions github-actions bot added area:docs Documentations comp:manager Related to Manager component comp:client Related to Client component comp:cli Related to CLI component require:db-migration Automatically set when alembic migrations are added or updated labels Dec 5, 2024
@jopemachine Graphite App
Copy link
Member Author

jopemachine commented Dec 5, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions github-actions bot added the size:XL 500~ LoC label Dec 5, 2024
@jopemachine jopemachine mentioned this pull request Dec 5, 2024
Closed
2 tasks
@jopemachine jopemachine added this to the 24.12 milestone Dec 5, 2024
@jopemachine jopemachine added type:feature Add new features type:refactor Refactor codes or add tests. labels Dec 5, 2024
@jopemachine jopemachine marked this pull request as ready for review December 5, 2024 06:03
This was referenced Dec 5, 2024
Open
Closed
@jopemachine
Copy link
Member Author

jopemachine commented Dec 5, 2024 •
edited
Loading

@HyeockJinKim How can we specifically implement a length restriction?
We need to decide whether the length restriction should be configurable, whether older logs in the status_history_log list should simply be deleted when the limit is exceeded, and whether a warning log should be printed during the deletion process.

This was referenced Dec 5, 2024
Open
Open
jopemachine
jopemachine commented Dec 6, 2024
View reviewed changes
src/ai/backend/manager/models/utils.py
for item in reversed(status_history_log):
if item["status"] == status:
return dtparse(item["timestamp"])
return None
Copy link
Member Author

Choose a reason for hiding this comment

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

@HyeockJinKim reversed() return Iterator, so it allows iterating from the end of the list without copying the entire list, eliminating the need to use range().
But we should use reversed() alone, not list(reversed(...)) which copy the entire list.

@jopemachine jopemachine force-pushed the topic/12-05-feat_replace_sessions_kernels_s_status_history_s_type_dict_with_list_ branch 4 times, most recently from 656df34 to dceabcc Compare December 10, 2024 00:38
@jopemachine
Copy link
Member Author

Broken CI will be fixed in #3235.

@jopemachine jopemachine force-pushed the topic/12-05-feat_replace_sessions_kernels_s_status_history_s_type_dict_with_list_ branch from 850165f to 98c21c0 Compare December 13, 2024 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HyeockJinKim HyeockJinKim Awaiting requested review from HyeockJinKim

Assignees

@jopemachine jopemachine

Labels
area:docs Documentations comp:cli Related to CLI component comp:client Related to Client component comp:manager Related to Manager component require:db-migration Automatically set when alembic migrations are added or updated size:XL 500~ LoC type:feature Add new features type:refactor Refactor codes or add tests.
Projects
None yet
Milestone
24.12
Development

Successfully merging this pull request may close these issues.

Change the status_history column in the kernels and sessions tables to list.
1 participant
@jopemachine