Skip to content
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

Tickets/DM-16797 #72

Merged
merged 1 commit into from Dec 22, 2018
Merged

Tickets/DM-16797 #72

merged 1 commit into from Dec 22, 2018

Conversation

natelust
Copy link
Contributor

No description provided.

Copy link
Member

@TallJimbo TallJimbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the decorator isn't doing what you want it to do; otherwise looks fine.

optional=True,
doc=("Templated name of string, used to set name "
"field according to a shared substring at "
"configuration time"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe change "configuration time" to "when formatTemplateNames is called", to be a bit clearer.

@@ -284,6 +295,15 @@ class ResourceConfig(pexConfig.Config):
doc="Minimal number of cores needed by task.")


def _templateNameStorageDecorator(func):
storedParamsDict = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this variable ends up effectively being a class attribute, by virtue of being a local variable in a closure defined at class scope. I think what you want is a new instance attribute, which I think you can do by just dropping the decorator and doing something like the following in formatTemplateNames directly:

if not hasattr(self, "_storedParamsDict"):
    self._storedParamsDict = {}
self._storedParamsDict.update(templateParamsDict)
templateParamsDict.update(self._storedParamsDict)

A completely different (and possibly cleaner approach) would be to use Formatter.parse to skip substitution for fields that don't actually have the parameters given in a particular call.

@@ -294,6 +314,12 @@ class PipelineTaskConfig(pexConfig.Config):
quantum = pexConfig.ConfigField(dtype=QuantumConfig,
doc="configuration for PipelineTask quantum")

@_templateNameStorageDecorator
def formatTemplateNames(self, templateParamDict):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend renaming this to substituteDatasetNames for consistency with the command-line argument.

Add the ability to specify a templated string which may be used to
format the name of PipelineTask Config fields.
@natelust natelust merged commit bd0dee3 into master Dec 22, 2018
@timj timj deleted the tickets/DM-16797 branch April 13, 2022 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants