Introduce ServerKernelManager class

[kevin-bates]

Per this comment/response, this pull request introduces a ServerKernelManager class which is instantiated by default within the AsyncMappingKernelManager for each managed instance. Currently, ServerKernelManager derives from jupyter_client's AsyncIOLoopKernelManager, thereby preserving existing functionality.

Introducing this class allows the Server to use this implementation to provide server-specific functionality like kernel-based events, the potential for highly available kernels, etc., and re-enforces the notion that the KernelManager class (and corresponding functionality) is the purview of the application.

Applications bringing their own subclass of AsyncIOLoopKernelManager should update their implementations to derive from ServerKernelManager or risk missing functionality that ultimately is associated with ServerKernelManager.

With this PR the activity-tracking attributes that were previously patched onto the current instance are formally defined traits. As a result, the execution_state's initial value is "initializing", which will be set to "starting" upon the completion of the start_kernel() method. If this complicates state management too much, we can have the initial state be "starting", but I felt "initializing" was more correct.

Note that this PR does NOT introduce a similarly named class relative to MappingKernelManager. This simplifies support and serves as an impetus for applications/server extensions to switch to using the async kernel management.

[codecov]

Codecov Report

Base: 39.29% // Head: 80.00% // Increases project coverage by +40.71% 🎉

Coverage data is based on head (d660edf) compared to base (222e713).
Patch coverage: 93.33% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1101       +/-   ##
===========================================
+ Coverage   39.29%   80.00%   +40.71%     
===========================================
  Files          68       68               
  Lines        8006     8019       +13     
  Branches     1585     1587        +2     
===========================================
+ Hits         3146     6416     +3270     
+ Misses       4664     1182     -3482     
- Partials      196      421      +225     

Impacted Files	Coverage Δ
jupyter_server/services/kernels/kernelmanager.py	81.42% <93.33%> (+56.90%)	⬆️
jupyter_server/_tz.py	100.00% <0.00%> (+5.88%)	⬆️
jupyter_server/serverapp.py	80.03% <0.00%> (+17.04%)	⬆️
jupyter_server/auth/decorator.py	89.65% <0.00%> (+17.24%)	⬆️
jupyter_server/services/security/handlers.py	100.00% <0.00%> (+17.64%)	⬆️
jupyter_server/extension/application.py	73.12% <0.00%> (+18.06%)	⬆️
jupyter_server/services/config/manager.py	100.00% <0.00%> (+18.75%)	⬆️
jupyter_server/extension/config.py	72.22% <0.00%> (+22.22%)	⬆️
jupyter_server/extension/manager.py	92.68% <0.00%> (+22.92%)	⬆️
jupyter_server/gateway/gateway_client.py	78.28% <0.00%> (+25.38%)	⬆️
... and 40 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Zsailer]

As a result, the execution_state's initial value is "initializing", which will be set to "starting" upon the completion of the start_kernel() method. If this complicates state management too much, we can have the initial state be "starting", but I felt "initializing" was more correct.

I know this is merely semantics and probably not worth debating too much, but wouldn't it be more accurate/appropriate to allow_none=True and make the initial state None. "initializing" seems more appropriate when "pre_start_kernel" is called.

Until pre_start_kernel happens, there shouldn't be an execution_state, because there is no kernel backing the kernel_manager yet.

[Zsailer]

Great stuff, @kevin-bates! Just one minor comment. Otherwise, this looks good to me! Thank you for working on this PR!

[Zsailer]

BTW, the coverage report is a little off here. I don't think we should block the merging of this PR based on coverage.

[kevin-bates]

wouldn't it be more accurate/appropriate to allow_none=True and make the initial state None. "initializing" seems more appropriate when "pre_start_kernel" is called.

I totally agree @Zsailer - good idea. I also removed the implementations for last_activity's default value (since it should not have an initial value) and start_kernel() (inadvertently left in to ensure override was taking place).

[blink1073]

Thanks both! I'll cut a new RC now, tentatively our last one