-
Notifications
You must be signed in to change notification settings - Fork 31
[uss_qualifier/resources] Update naming and documentation for ResourceModifyingResource #1474
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
Changes from 2 commits
afc93ca
8ad1a4d
cb3c10b
2fb5507
c75b95d
44881aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| from .noop import NoOpResource as NoOpResource | ||
| from .test_exclusions import TestExclusionsResource as TestExclusionsResource | ||
| from .test_modifier import TestModifierModifierResource as TestModifierModifierResource | ||
| from .test_modifier import TestModifierResource as TestModifierResource | ||
| from .test_modifier import ( | ||
| NumberGeneratorModifierResource as NumberGeneratorModifierResource, | ||
| ) | ||
| from .test_modifier import NumberGeneratorResource as NumberGeneratorResource |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,15 +41,19 @@ def is_type(self, resource_type: str) -> bool: | |
|
|
||
|
|
||
| ResourceType = TypeVar("ResourceType", bound=Resource) | ||
| SpawnKeyType = TypeVar("SpawnKeyType") | ||
|
|
||
|
|
||
| class ResourceModifier[SpecificationType: ImplicitDict, ResourceType]( | ||
| Resource[SpecificationType], ABC | ||
| ): | ||
| """A specifc type of resources that can return adjusted an resource that shall unique based on a specifc 'index'. | ||
| The underlying resource shall be a dependency named 'base_resource'. | ||
| class ResourceModifyingResource[ | ||
| SpecificationType: ImplicitDict, | ||
| SpawnKeyType, | ||
| ResourceType: Resource, | ||
| ](Resource[SpecificationType], ABC): | ||
| """Resource capable of spawning ResourceType resources by modifying a template/base | ||
| ResourceType resource according to a desired key, such as an index. | ||
|
|
||
| Concrete subclass must implement 'adjust' as needed. | ||
| Useful for deconflicting multiple copies of a resource so many different variants of the same | ||
| test can be performed without conflicting with each other. | ||
| """ | ||
|
|
||
| _spec: SpecificationType | ||
|
|
@@ -65,12 +69,17 @@ def __init__( | |
| self._spec = specification | ||
| self.base_resource = base_resource | ||
|
|
||
| def _modified_resource_origin(self, modification_name: str) -> str: | ||
| """Method that should be used to determine the origin of a resource spawned by this resource.""" | ||
| return f"Modification {modification_name} of {self.base_resource.resource_origin} by {self.resource_origin}" | ||
|
|
||
| @abstractmethod | ||
| def adjust(self, index: int) -> ResourceType: | ||
| """ | ||
| Return a new instance of the base resource, modified to be unique based on 'index' value. | ||
| def modify(self, key: SpawnKeyType) -> ResourceType: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it needed to allow for a dynamic type there? I would assume thoses ResourceModifyingResource will only be used by generators, and we don't want generators to need to know various types, just to use 'modify' in a generic way. Using an int mean we could use an simple index, or use
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this object is a generic resource-modifying resource, then it should support all the reasonable use cases when a resource produces other resources from a template resource. We could narrow its scope to IndexBasedResourceModifyingResource and thereby limit the key to an int, but the generic key allows more use cases and doesn't require any additional development to do so. I would expect modifiers intended specifically for iteration parallelization via index would definitely use int. But, I'm not sure why we would need to limit to a specific key type here rather than using the appropriate key type when this tool is used. Basically, this is a map that maps values (probably from an iterator) to corresponding resources. I'm not sure why we would want to intentionally limit it to int -> resource rather than T -> resource when the latter supports the former with no additional code, but also other Ts as well.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, ours comments crossed and indeed it make sense to have it very generic at this level, but it would make sense to have a specific subtype for iteration parallelization then? Either the subtype enforce the type (to be an int or something else), or parallelizer need to handle multiple cases (like ignoring ResourceModifyingResource it cannot handle, handle multiple type (like str() is easy), and that seems complex)* (*That in future PR, but right now passing and '.modifing()' resources to have them parallelizable is quite simple and clean: b7d58e8#diff-d44fbbd1adab54bf42eb54d25ec991f1e3f2a22f9cfad7aa517ec7a374e0c09eR106 )
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would expect subtype or not to be independent of parallelization. Action generators generate a sequence (perhaps set, in the future) of actions and we're just looking to generate resources to go along with those actions that will reduce interference between actions. It would be valuable to do this even without parallelization because then failing iteration N has a lower chance of causing iterations after N to fail also. The only thing parallelization would do differently is enumerate all those actions up front and then dispatch them to workers with some degree of simultaneity rather than sequentially.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes I do agree, but I think the question is still valid: Since I thought about a subtype, and "users" can just do something like that: if isinstance(ResourceModifyingResourceUsingInt, rmr):
new_resource = rmr.modify(index)
elif isinstance(ResourceModifyingResourceUsingUSSPair, rmr):
new_resource = rmr.modify({base_uss, target_uss})There are also other solutions:
What did you have in mind in practice?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Action generators will presumably be the main users/callers as they are generally the ones that need to produce a large number of variants of a resource. We want to tell the action generator that, instead of passing resource-modifying resource X through to each of its actions, it should instead invoke X's One way to implement could be to allow the test designer to specify a list of resources that should be treated as ResourceModifyingResources by the action generator (this could take the form of a mapping of "resource name in the pool available to the action generator" to "resource name in the pool available to the action"). The resource generator would prepare each action in the same way it does now, except any resource listed as a ResourceModifyingResource it would not move to the action's pool, but instead call Having written the above, it occurs to me that ResourceModifyingResource is perhaps even more specific than we need -- it seems like what we really want is a ResourceGeneratingResource (a resource that generates resources based on a key), and a ResourceModifyingResource is a special case of ResourceGeneratingResource that performs its generation by modifying a base resource.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think we want to do that: That would generate typing errors and tracktraces no?
Okay, but how should we indicate when to pass or modify? (Maybe check next PRs in the original tree since it's a practical implementation, even with wrong naming)
Is there really a difference and/or distinction needed? 'How' it's generated is not really important for 'callers' no?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Added a commit to add a ResourceProvidingResource base class. |
||
| """Spawn a new resource formed by modifying the template/base resource according to the provided key. | ||
|
|
||
| Different `key` values generally produce different variants; the same `key` should produce equivalent results. | ||
| """ | ||
| pass | ||
| raise NotImplementedError() | ||
|
|
||
|
|
||
| class MissingResourceError(ValueError): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Shouldn't it be the opposite ? A different
keymust produce different variants; the same may produce equivalents result?I would assume that the spirit is to ensure different resources when needed because that more important that having the same values no?
(We can however have a 'must' for the same key -> same result)
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.
I'm not sure I understand the question; the change says "different key values -> different variants" + "same key -> equivalent results" -- the same thing as the original, just using "key" instead of "index".
One substantive difference is the removal of "(unique)" because that will probably often be the case, but there's no reason it necessarily needs to be in any case where we want a resource that spawns resources based on a template resource.
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.
That indeed about the "(unique)" removed and the addition of 'generally': since the final goal is to be able to have resources that can be used in parallel without conflicts, I would expect that those resources generator, by 'contract' return different variants in every case, not in most of the cases.
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.
The use case prompting the creation of this resource needs unique variants in all cases and should always produce equivalent results with equivalent keys. But the way this resource is defined allows it to be used in more general ways. Just because one use of a tool has certain requirements doesn't mean those requirements need to be imposed at the tool level rather than the tool-usage level. For instance, suppose we wanted to iterate over all ordered {uss1, uss2} pairs of participants, but a test needed a token that corresponded with the unordered pair of {uss1, uss2} (so {uss1, uss2} has a different token resource than {uss1, uss3}, but the same token resource as {uss2, uss1}). The suite may still be iterating over ordered pairs, but the resource producing modified versions of the underlying token resource may produce the same modified resource for multiple keys.
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, but should we then define an sub-type of the base type than enforce this? I would expect instances used in 'parallel generator' to follow stricter rules (This probably answer the type question bellow as well)
(True, but in that case since it's unordered, it would be an equal key ({uss1,uss2}=={uss2,uss1}) even if it's has different values. But that don't mean other cases wouldn't exists.)
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.
I wouldn't expect we'd need to. A good test design will produce test scenarios that don't interfere with each other. Not interfering sometimes means "unique" modified resources (a necessary but not sufficient condition to deconflict geospatial resources), but not always (like in the case of iterating over ordered pairs but only needing unique unordered pairs for the token resource). But if we did need "unique" modified resources, I'd expect that to be introduced along with the thing that actually needs that constraint. The content in this PR (and #1465) doesn't need uniqueness -- the concept of stamping out modified copies of a template resource based on a key is a good standalone concept that doesn't need the additional constraint to work.
No, the key would be ordered as the test scenarios (or whatever is being primarily iterated) are distinguishable based on order in the example, but the particular resource being generated by modifying a base resource doesn't depend on order. To elaborate on the example: suppose we have a test scenario that has two roles and uses a token resource that needs to be "unique" according to participants in the scenario, but not according to what roles they're playing. The action generator would iterate over every (role1, role2) assignment of the {uss1, uss2, uss3, ...} participants when instantiating test scenarios. And, each test scenario would need a token resource based on some template token resource. But, the token resource needed would only depend on the participants rather than role assignments, so the modified token resources spawned would not be unique. At the same time, geospatial deconfliction is needed, so every set of flight intents would need to be "unique".
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 - that move the "responsibility" up to test definitions to only combine things that works together without an explicit 'contract', but that valid.