Implement lazy directives for Ramble - #1756
linsword13 wants to merge 4 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors Ramble's directive system to support lazy evaluation of directives upon first access using a new DirectiveDictDescriptor descriptor. It removes the need for explicit class-to-instance attribute conversion and updates various base classes to use DirectiveMeta directly as their metaclass. Additionally, comprehensive tests are added to verify lazy evaluation, inheritance, and attribute isolation. The review feedback highlights a critical issue where instance-level modifications to these lazily evaluated directive dictionaries would be lost during cloning or copying. It is recommended to dynamically copy all evaluated directive dictionaries from self.__dict__ to the cloned or copied instances in both ApplicationBase.clone() and ObjectMixin.copy().
Ramble Performance Test MetricsResults produced with commit: 85adeac
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1756 +/- ##
===========================================
+ Coverage 94.07% 94.08% +0.01%
===========================================
Files 370 370
Lines 37399 37554 +155
===========================================
+ Hits 35182 35333 +151
- Misses 2217 2221 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors Ramble's directive system to support lazy evaluation on first access, introducing the DirectiveDictDescriptor class and removing the eager conversion of class attributes to instance attributes across all base classes. A comprehensive test suite has been added to verify lazy evaluation, inheritance, and isolation. The review feedback highlights critical inheritance issues where using getattr on classes or objects can mistakenly retrieve parent class attributes (such as evaluated directive dictionaries or preferred versions) instead of checking the subclass's own __dict__ directly, which would bypass subclass directive execution or raise incorrect errors.
1fadb66 to
87a882c
Compare
|
/gemini review |
7efbad1 to
2b398d1
Compare
|
/gemini review |
rfbgo
left a comment
rfbgo
left a comment
There was a problem hiding this comment.
Two random questions:
- The bot reports a slow down from this. Is it it true? If so I think there are some places we might be able to improve perf slightly (eg optimized copies)
- I asked gemini to review this PR wrt "leaking" (it says it will leak). I kind of expect this is something you were already careful about, but I wanted to double check we believe this is indeed safe?
| DirectiveMeta._directives_to_be_executed = [ | ||
| (n, fn) | ||
| for n, fn in DirectiveMeta._directives_to_be_executed | ||
| if fn != arg |
There was a problem hiding this comment.
Can this be expressed safely as is not?
| ) -> Callable[..., Any]: | ||
| """Decorator for Ramble directives.""" | ||
| if dicts is None or dicts == (): | ||
| dicts_tuple: Tuple[str, ...] = () |
There was a problem hiding this comment.
I don't really understand what this is doing, can you help me understand? Elsewhere this stuff is keyed on func_name, right? It feels like this could accidentally turn stuff into a noop?
| def preferred_version(self, value: Optional[ObjectVersion]): | ||
| self._preferred_version = value | ||
|
|
||
| def _copy_evaluated_directives(self, target): |
There was a problem hiding this comment.
Is this able to retain preferred_version too?
|
|
||
| def _pop_default_args() -> dict: | ||
| return DirectiveMeta._default_args.pop() | ||
| _UNSET = object() |
There was a problem hiding this comment.
Something odd and interesting is leaking in an info print, I (well.. gemini...) thinks it's this:
ramble info --type modifier ...
...
default_mode:
<object object at 0x1052e1e50>
Great questions: For 1. it does seem to cause a slowdown on some perf tests. One reason is that previously the directive initialization cost are part of the startup, which wasn't counted by the benchmarking. And now with the lazy init, these init times are now part of the operations that are being benchmarked. So this is more or less an accounting thing. Other factors include the more works that are needed during copy (as we can no longer rely on the class-level attributes.) I should do a more thorough analysis though to see if any improvements are possible, and at least have a better breakdown of the timing. For 2. I did try to look for leaking, but I will do another pass to see where I missed. |
This is inspired by Spack's lazy directive (spack/spack#51881). * Add in the dict descriptor (as a non-data descriptor) to support the lazy initiailization * Remove the `convert_class_attributes` since now the lazy execution allows us to perform copy on first access * Update the various directives to support the new semantics. Specifically, it now requires listing out all mutated dicts of a directive * Also unify the various language meta directives, so now they are all essentially DirectiveMeta * Update the mirror test since before it implicitly relied on the mutation happening both at the class and the instance level Signed-off-by: Lin Guo <linsword13@gmail.com>
Signed-off-by: Lin Guo <linsword13@gmail.com>
Signed-off-by: Lin Guo <linsword13@gmail.com>
2b398d1 to
73a3649
Compare
- Map default_mode to public descriptor default_usage_mode in info.py to prevent _UNSET leakage. - Preserve class-level attribute values matching directive names in DirectiveMeta via _class_directive_values. - Deduplicate directives by callable identity in DirectiveMeta and eagerly execute directives without target dictionaries (dicts=()). - Add execution plan caching (_execution_plan_cache) in DirectiveMeta for O(1) membership checks. - Scope directive descriptors to declared language types via _type_scoped_dicts to avoid leaking inapplicable descriptors. - Fix identity comparison in remove_directives (fn is not arg). - Clean up unused system attribute declarations in system-base/base_class.py. - Retain selective deepcopy in ObjectMixin._copy_evaluated_directives to ensure cloned experiment isolation while fast-pathing empty and primitive values. - Eliminate ad-hoc _preferred_version state, _preferred_version_class, and DirectiveMeta._current_directive; derive preferred_version as a computed property from known_versions and attach _defining_class directly to ObjectVersion. Signed-off-by: Lin Guo <linsword13@gmail.com>
73a3649 to
85adeac
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
2 participants
This is inspired by Spack's lazy directive (spack/spack#51881).
convert_class_attributessince now the lazy execution allows us to perform copy on first access