backend.ai
backend.ai copied to clipboard
Let's group string-literal `KernelEvent.reason` values into a manageable format
Is your feature request related to a problem? Please describe.
Each Kernel-Lifecycle-EventArgs class has a reason
field to briefly describe the source of the lifecycle event. For example, when terminating a kernel, a KernelTerminatingEvent
object is published with the reason
field inherited from its parent class KernelTerminationEventArgs
.
https://github.com/lablup/backend.ai/blob/c33063ec7f567eb9f36821a19348d910d2e27c22/src/ai/backend/common/events.py#L194-L197
https://github.com/lablup/backend.ai/blob/c33063ec7f567eb9f36821a19348d910d2e27c22/src/ai/backend/common/events.py#L266-L269
There are several values used as a default reason such as user-requested
or self-terminated
, and the problem is that it is hard to manage since they are just string literals.
https://github.com/lablup/backend.ai/blob/c33063ec7f567eb9f36821a19348d910d2e27c22/src/ai/backend/manager/api/session.py#L1491-L1498
Describe the solution you'd like I suggest managing each of the default reason values as a constant variable. Having an enum class can be an option, but it will cause a problem in understanding raw data from a database that stores enum values in index-like formats(0, 1, ...).
Describe alternatives you've considered
Additional context Here is an example:
class KernelLifecycleEventReason:
USER_REQUESTED = "user-requested"
FORCE_TERMINATED = "force-terminated"
...
await root_ctx.registry.destroy_session(
functools.partial(
root_ctx.registry.get_session_by_session_id,
event.session_id,
),
forced=False,
reason=event.reason or KernelLifeCycleEventReason.KILLED_BY_EVENT,
)
Dear rapsealk,
I'm finding string-literal KernelEvent.reason
for this issue.
Meanwhile, I saw several reasons about SessionEvent, AgentEvent, etc.
If I make a PR for this issue, Could I include reasons for not only KernelEvent, but other Event?
@qkoo0833 Oh, I just found your comment now. First of all, thanks for your support. I think it would be better to separate those tasks into different issues for each.
@rapsealk Thank you for your review. I made additional commit that include only KernelEvent.reason
and add change log.
PR#700 make conflict so I closed #700. And I make new PR to this issue.