Move ownership of Variables from Application to Profile#759
Open
adriaan42 wants to merge 1 commit into
Open
Conversation
adriaan42
force-pushed
the
adriaan/variable-bug
branch
from
a531df0 to
f17c247
Compare
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR refactors variable handling throughout tuned by moving variables from plugin-level storage to per-instance variables. Profile/Factory/Loader/Merger are updated to thread and merge Variables per-load, Instance stores per-instance Variables, Plugin/Repository/Application/Manager/Daemon signatures and wiring were changed to pass variables, sysfs and command expansions use instance variables, and tests were updated to match new signatures. ChangesPer-Instance Variable Refactoring
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tuned/profiles/loader.py (1)
26-27: 💤 Low value
_create_profileappears unused.This method creates a
Profiledirectly, but_load_profileat line 80 usesself._profile_factory.create()instead. If this is intentional (e.g., for subclass overriding), consider documenting the design intent. Otherwise, it may be dead code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tuned/profiles/loader.py` around lines 26 - 27, _dead code: _create_profile is defined but never used; either remove it or integrate/document its intended override use. If intended for subclassing, add documentation on the class or the _create_profile method stating that subclasses should override _create_profile to customize Profile construction and change _load_profile to call self._create_profile(...) instead of self._profile_factory.create(...). If not intended, delete the _create_profile method and rely solely on the _profile_factory.create(...) usage found in _load_profile. Ensure references to tuned.profiles.profile.Profile are adjusted or removed accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tuned/plugins/base.py`:
- Around line 251-253: The code currently aliases and mutates os.environ via the
local variable environ (environ = os.environ;
environ.update(instance._variables.get_env())), which pollutes the global
process environment; instead create a fresh per-call environment dict (e.g., env
= os.environ.copy()), update that with instance._variables.get_env(), and use
that env only for the subprocess/script invocation (where arguments is used) so
you do not modify os.environ globally. Ensure you replace uses of the aliased
environ with the new env when launching the process.
---
Nitpick comments:
In `@tuned/profiles/loader.py`:
- Around line 26-27: _dead code: _create_profile is defined but never used;
either remove it or integrate/document its intended override use. If intended
for subclassing, add documentation on the class or the _create_profile method
stating that subclasses should override _create_profile to customize Profile
construction and change _load_profile to call self._create_profile(...) instead
of self._profile_factory.create(...). If not intended, delete the
_create_profile method and rely solely on the _profile_factory.create(...) usage
found in _load_profile. Ensure references to tuned.profiles.profile.Profile are
adjusted or removed accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 1d8d090d-374f-4e7b-bbdd-6fc0a800ce41
📒 Files selected for processing (15)
tests/unit/plugins/test_base.pytests/unit/profiles/test_loader.pytests/unit/profiles/test_merger.pytests/unit/profiles/test_profile.pytuned/daemon/application.pytuned/daemon/daemon.pytuned/plugins/base.pytuned/plugins/instance/instance.pytuned/plugins/plugin_sysfs.pytuned/plugins/repository.pytuned/profiles/factory.pytuned/profiles/loader.pytuned/profiles/merger.pytuned/profiles/profile.pytuned/units/manager.py
adriaan42
changed the title
Currently, there is one global Variables object, as member of the global Application. When loading profiles, variables are added to it, but never removed. So when switching profiles, there are still variables present from the previous profile. With this commit, the Variables object is now owned by the Profile. It is created by the profile loader, so that more complex laod scenarios (multiple profiles, potentially with includes) still only use one Variables object. Signed-off-by: Adriaan Schmidt <adriaan.schmidt@siemens.com>
adriaan42
force-pushed
the
adriaan/variable-bug
branch
from
f17c247 to
b863272
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/plugins/test_base.py (1)
106-116: ⚡ Quick winExercise actual instance-variable expansion in these tests.
These cases wire the new parameter through, but they still use literal command values, so they would pass even if runtime expansion never read
instance.variables. Seed aVariablesobject and use${...}in the options here, and intest_get_current_value, so this PR gets a regression test for the behavior it changes.This aligns with the PR objective to move runtime expansion to per-instance variables.
Also applies to: 135-141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/plugins/test_base.py` around lines 106 - 116, The tests currently pass literal option values so they don't validate per-instance variable expansion; update the test cases (those calling create_instance and invoking _execute_all_non_device_commands and test_get_current_value) to seed a profiles.variables.Variables() instance with the appropriate key/value and change the option maps to use a ${variable_name} reference (e.g., {'size':'${size}'} and {'device_setting':'${device_setting}'}), then assert that after calling _execute_all_non_device_commands (and the device command test) the plugin resolved those variables into the expected concrete values (e.g., _size == 'XXL' and device setting == '010'); reference create_instance and _execute_all_non_device_commands to locate where to pass the Variables instance and where expansion should occur, and apply the same pattern to the similar tests around the 135-141 region.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/plugins/test_base.py`:
- Around line 106-116: The tests currently pass literal option values so they
don't validate per-instance variable expansion; update the test cases (those
calling create_instance and invoking _execute_all_non_device_commands and
test_get_current_value) to seed a profiles.variables.Variables() instance with
the appropriate key/value and change the option maps to use a ${variable_name}
reference (e.g., {'size':'${size}'} and {'device_setting':'${device_setting}'}),
then assert that after calling _execute_all_non_device_commands (and the device
command test) the plugin resolved those variables into the expected concrete
values (e.g., _size == 'XXL' and device setting == '010'); reference
create_instance and _execute_all_non_device_commands to locate where to pass the
Variables instance and where expansion should occur, and apply the same pattern
to the similar tests around the 135-141 region.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 30d9634b-d09c-403f-86e1-5c3e26c0301c
📒 Files selected for processing (15)
tests/unit/plugins/test_base.pytests/unit/profiles/test_loader.pytests/unit/profiles/test_merger.pytests/unit/profiles/test_profile.pytuned/daemon/application.pytuned/daemon/daemon.pytuned/plugins/base.pytuned/plugins/instance/instance.pytuned/plugins/plugin_sysfs.pytuned/plugins/repository.pytuned/profiles/factory.pytuned/profiles/loader.pytuned/profiles/merger.pytuned/profiles/profile.pytuned/units/manager.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/profiles/test_profile.py
🚧 Files skipped from review as they are similar to previous changes (11)
- tuned/plugins/plugin_sysfs.py
- tuned/plugins/instance/instance.py
- tuned/daemon/daemon.py
- tuned/units/manager.py
- tuned/daemon/application.py
- tuned/plugins/base.py
- tests/unit/profiles/test_loader.py
- tuned/profiles/factory.py
- tests/unit/profiles/test_merger.py
- tuned/profiles/merger.py
- tuned/plugins/repository.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While working on #758, I noticed a bug in the way variables are handled:
Currently, there is one global
Variablesobject, as member of the globalApplication. When loading profiles, variables are added to it, but never removed. So when switching profiles, there are still variables present from the previous profile.To demonstrate, use this debug print, with two profiles:
variable1:variable2:Then use
tuned-adm profileto switch between them, and note how the debug output will contain not only variables of the current, but also of the previously loaded profile:My proposal is to have the
Variablesobject owned by theProfile. It is created by the profile loader, so that more complex load scenarios (multiple profiles, potentially with includes) still only use oneVariablesobject.An alternative would be to simply clear variables when loading a new profile, but that would break my #758, where (when restoring a snapshot) a second profile is loaded, which can fail, and therefore does not work well with
Variablesas global state.