-
Notifications
You must be signed in to change notification settings - Fork 122
Target Manager Parameter Updates #1109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
marcbonnici
commented
Jun 23, 2020
- Fix the implementation of Target Manager parameters
- Enable the configuration of the target_info cache
- Fix typo
wa/framework/target/manager.py
Outdated
| def _init_target(self): | ||
| # Import here to prevent circular import | ||
| # pylint: disable=wrong-import-position,cyclic-import | ||
| from wa.framework.target.descriptor import (get_target_description, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the cycle here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this again, I think I moved the wrong import and should have moved the new TargetManger import in the descriptor to be more obvious.
But essentially the cycle here is that the TargetManager needs access to the target descriptor methods as before, however this PR also adds the requirement for the target descriptor to access the TargetManagers parameters so it can add them to the target descriptor for filtering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
however this PR also adds the requirement for the target descriptor to access the TargetManagers parameters
But those are accessed via an argument passed into instantiate_target when called. I don't see the load-time descriptor.py --> manager dependency. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, wait, never mind; just saw the update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so issue here is that TargetManager parameters are being populated inside TargetDescription.get_descriptions(). This breaks conceptual integrity -- TargetDescription should not know about TargetManager; it only describes a target.
Rather than doing this, I would pass the manager parameters as arguments to list_target_descriptions and add them to each description there.
The target manager parameters have not been previously been used as they have been missing the required plumbing. Allow configuration of the TargetManager via the `device_config` section.
Allow disabling of the usage of the target_info cache. This is useful when running on a one-off target and there is no benefit of using caching the target information.