Skip to content

Conversation

@vkarak
Copy link
Contributor

@vkarak vkarak commented May 13, 2023

The problem was that when variables from base classes where lowered in the current namespace as ShadowVars their value was not deep copied as in the past. As a result, when a mutable variable was updated in a subclass body, the original copy of the base class was being changed and this was visible to any new class inheriting it.

The solution is much more complicated as simply doing a deep copy of the original value, due to aliases. If we lower the root variable and simply deep copy its value, its alias still refers to the original one. In this case, for example, env_vars behaviour would have been fixed but not that of variables. As a result, whenever a variable is lowered, all of its aliases are also lowered. We essentially recreate the aliases as ShadowVars pointing to the ShadowVar of the lowered root variable.

Finally, a couple of tweaks were needed for deprecated aliases:

  1. We need to replace the reference in the root variable to point to the deprecated version of the alias
  2. We suppress any deprecation warnings whenever a deprecated alias is lowered.

Closes #2848.

@vkarak vkarak added this to the ReFrame 4.3 milestone May 13, 2023
@vkarak vkarak requested review from ekouts and teojgo May 13, 2023 19:47
@vkarak vkarak self-assigned this May 13, 2023
@codecov
Copy link

codecov bot commented May 13, 2023

Codecov Report

Patch coverage: 97.61% and project coverage change: +0.04 🎉

Comparison is base (f8245bc) 86.91% compared to head (2936403) 86.95%.

❗ Current head 2936403 differs from pull request most recent head 5cf262f. Consider uploading reports for the commit 5cf262f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2880      +/-   ##
==========================================
+ Coverage   86.91%   86.95%   +0.04%     
==========================================
  Files          60       60              
  Lines       11509    11546      +37     
==========================================
+ Hits        10003    10040      +37     
  Misses       1506     1506              
Impacted Files Coverage Δ
reframe/core/variables.py 95.68% <96.42%> (+0.26%) ⬆️
reframe/core/meta.py 99.13% <100.00%> (+0.03%) ⬆️

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

@vkarak vkarak force-pushed the bugfix/default-mutable-vars branch from c87dcd9 to 5cf262f Compare May 13, 2023 19:54
@vkarak vkarak linked an issue May 16, 2023 that may be closed by this pull request
@vkarak vkarak merged commit f13ae95 into reframe-hpc:master May 16, 2023
@vkarak vkarak deleted the bugfix/default-mutable-vars branch May 16, 2023 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Subclasses can modify default mutable variables

2 participants