|SUCCESS| [v4, 7/7] dts: separate Linux session into interface and logic

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163516

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-01

# DPDK Patch Review

commit: 20260430210917.25186-1-dmarx at iol.unh.edu (series)
Author: Dean Marx <dmarx at iol.unh.edu>
Subject: [PATCH v4 1/7] dts: move exception module from framework to API

This patch series moves several framework modules to the API to support
direct importing from test suites. The patches rename and relocate
modules to support the goal that test suite imports should come from the
API.

## Summary of Changes

Patch 1/7 moves exception module from framework to API. All imports of
exception are updated throughout the codebase.

> diff --git a/dts/framework/exception.py b/dts/api/exception.py
> similarity index 100%
> rename from dts/framework/exception.py
> rename to dts/api/exception.py

Does this file move introduce any changes to the exception.py contents?
The rename appears clean but the diff shows "100%" similarity. Can this
be verified with --find-renames or similar to ensure no content changes
occurred during the move?

> diff --git a/dts/api/artifact.py b/dts/api/artifact.py
> index 24a2b05063..7d04c7ab49 100644
> --- a/dts/api/artifact.py
> +++ b/dts/api/artifact.py
> @@ -47,7 +47,7 @@
>  from paramiko import SFTPClient, SFTPFile
>  from typing_extensions import Buffer
>  
> -from framework.exception import InternalError
> +from api.exception import InternalError

Across all files, do these import path changes maintain the same module
objects being imported? Has testing confirmed that no runtime import
errors occur with the new paths?

[ ... ]

commit: 20260430210917.25186-4-dmarx at iol.unh.edu
Author: Dean Marx <dmarx at iol.unh.edu>
Subject: [PATCH v4 4/7] dts: move testbed model from framework to API

> diff --git a/dts/framework/testbed_model/__init__.py b/dts/api/testbed_model/__init__.py
> similarity index 100%
> rename from dts/framework/testbed_model/__init__.py
> rename to dts/api/testbed_model/__init__.py

Does this move of the entire testbed_model directory preserve the
__init__.py exports? Can empty __init__.py files cause import issues if
modules previously relied on implicit namespace package behavior?

> diff --git a/dts/framework/testbed_model/capability.py b/dts/api/testbed_model/capability.py

Does moving capability.py and its dependencies create any circular import
issues? The file imports from framework.logger and api.testbed_model -
does the import order remain safe?

[ ... ]

commit: 20260430210917.25186-5-dmarx at iol.unh.edu
Author: Dean Marx <dmarx at iol.unh.edu>
Subject: [PATCH v4 5/7] dts: move test suite module from framework to API

> diff --git a/dts/framework/test_suite.py b/dts/api/test_suite.py
> similarity index 99%
> rename from dts/framework/test_suite.py
> rename to dts/api/test_suite.py
> @@ -34,8 +34,7 @@
>  from api.testbed_model.topology import Topology
>  from api.utils import to_pascal_case
>  from framework.config.common import FrozenModel
> -
> -from .logger import DTSLogger, get_dts_logger
> +from framework.logger import DTSLogger, get_dts_logger

Does changing from relative import (`.logger`) to absolute import
(`framework.logger`) cause any issues? Are there any test cases that
validate the logger is correctly initialized after this change?

> diff --git a/dts/framework/config/test_run.py b/dts/framework/config/test_run.py
> @@ -230,7 +230,7 @@ class TestSuiteConfig(FrozenModel):
>      @cached_property
>      def test_suite_spec(self) -> "TestSuiteSpec":
>          """The specification of the requested test suite."""
> -        from framework.test_suite import find_by_name
> +        from api.test_suite import find_by_name

Does moving find_by_name to api.test_suite affect its ability to
discover test suites? Does the discovery mechanism still work correctly
with the new import path?

[ ... ]

commit: 20260430210917.25186-6-dmarx at iol.unh.edu
Author: Dean Marx <dmarx at iol.unh.edu>
Subject: [PATCH v4 6/7] dts: move params directory from framework to API

> diff --git a/dts/framework/params/types.py b/dts/api/params/types.py
> @@ -12,8 +12,10 @@ def create_testpmd(**kwargs: Unpack[TestPmdParamsDict]):
>              params = TestPmdParams(**kwargs)
>  """
>  
> +from __future__ import annotations
> +
>  from pathlib import PurePath
> -from typing import TypedDict
> +from typing import TYPE_CHECKING, TypedDict

Does adding `from __future__ import annotations` here change any runtime
behavior? Is this addition necessary for the type checking imports, or
does it affect string annotation evaluation elsewhere?

> @@ -56,7 +58,9 @@ def create_testpmd(**kwargs: Unpack[TestPmdParamsDict]):
>      TXRingParams,
>      TxUDPPortPair,
>  )
> -from framework.params import Switch, YesNoSwitch
> +
> +if TYPE_CHECKING:
> +    from api.params import Switch, YesNoSwitch

Does moving Switch and YesNoSwitch behind TYPE_CHECKING guard affect
runtime usage? Are these types only used in annotations, or are they
instantiated at runtime anywhere?

> diff --git a/dts/api/testpmd/__init__.py b/dts/api/testpmd/__init__.py
> @@ -14,6 +14,8 @@
>      testpmd.close()
>  """
>  
> +from __future__ import annotations
> +
>  import functools
>  import re
>  import time
> @@ -56,7 +59,9 @@
>      TxOffloadConfiguration,
>      VLANOffloadFlag,
>  )
> -from framework.params.types import TestPmdParamsDict
> +
> +if TYPE_CHECKING:
> +    from api.params.types import TestPmdParamsDict

Does guarding TestPmdParamsDict import with TYPE_CHECKING cause issues
where it is used at runtime (e.g., in Unpack[TestPmdParamsDict])? Can
this create NameError when the code executes?

[ ... ]

commit: 20260430210917.25186-7-dmarx at iol.unh.edu
Author: Dean Marx <dmarx at iol.unh.edu>
Subject: [PATCH v4 7/7] dts: separate Linux session into interface and logic

> diff --git a/dts/api/testbed_model/linux_session.py b/dts/api/testbed_model/linux_session.py
> index 7307b2abe2..5bcbf1ce97 100644
> @@ -1,367 +1,41 @@
> -"""Linux OS translator.
> +"""Linux OS session interface.
>  
> -Translate OS-unaware calls into Linux calls/utilities. Most of Linux distributions are mostly
> -compliant with POSIX standards, so this module only implements the parts that aren't.
> -This intermediate module implements the common parts of mostly POSIX compliant distributions.
> +Extends the base :class:`~.os_session.OSSession` with methods specific to Linux nodes.
> +The concrete implementation containing all backend logic lives in the framework package.
>  """

Does splitting LinuxSession into an abstract interface (in API) and
concrete implementation (in framework) introduce any issues with method
resolution order? Are all abstract methods properly implemented in the
framework version?

> diff --git a/dts/api/testbed_model/node.py b/dts/api/testbed_model/node.py
> @@ -15,17 +15,19 @@
>  
>  from functools import cached_property
>  from pathlib import PurePath
> -from typing import Literal, TypeAlias
> +from typing import TYPE_CHECKING, Literal, TypeAlias
>  
>  from api.exception import ConfigurationError, InternalError
>  from framework.config.node import (
>      OS,
>      NodeConfiguration,
>  )
> +
> +if TYPE_CHECKING:
> +    from framework.linux_session import LinuxSession
>  from framework.logger import DTSLogger, get_dts_logger

Does guarding LinuxSession import with TYPE_CHECKING cause runtime
issues? The create_session() function returns instances of LinuxSession,
so is the import guard safe?

> @@ -211,7 +213,7 @@ def create_session(node_config: NodeConfiguration, name: str, logger: DTSLogger)
>      """
>      match node_config.os:
>          case OS.linux:
> -            return LinuxSession(node_config, name, logger)
> +            return "LinuxSession"(node_config, name, logger)

Does this code attempt to call the string "LinuxSession" as a
constructor? This will cause a TypeError at runtime since strings are not
callable. Should this be the actual LinuxSession class from
framework.linux_session?

> diff --git a/dts/framework/linux_session.py b/dts/framework/linux_session.py
> new file mode 100644
> index 0000000000..e5320b7fc4
> +++ b/dts/framework/linux_session.py
> @@ -0,0 +1,366 @@
> +class LinuxSession(PosixSession, LinuxSessionBase):
> +    """The implementation of non-Posix compliant parts of Linux."""

Does this class properly inherit from both PosixSession and
LinuxSessionBase? Are there any method name conflicts or diamond
inheritance issues with this multiple inheritance pattern?