backend.ai icon indicating copy to clipboard operation
backend.ai copied to clipboard

Let's group string-literal `KernelEvent.reason` values into a manageable format

Open rapsealk opened this issue 2 years ago • 4 comments

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, 
) 

rapsealk avatar Aug 23 '22 01:08 rapsealk

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 avatar Aug 27 '22 08:08 qkoo0833

@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 avatar Sep 01 '22 10:09 rapsealk

@rapsealk Thank you for your review. I made additional commit that include only KernelEvent.reason and add change log.

qkoo0833 avatar Sep 07 '22 12:09 qkoo0833

PR#700 make conflict so I closed #700. And I make new PR to this issue.

qkoo0833 avatar Sep 17 '22 06:09 qkoo0833