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

PortNamespace: do not set dynamic=False when valid_type=None #146

Merged
merged 1 commit into from
Jun 5, 2020

Conversation

sphuber
Copy link
Collaborator

@sphuber sphuber commented Feb 12, 2020

Fixes #135

When setting valid_type to anything but None the setter will
automatically also set dynamic to True, because this almost always
what one wants and so saves having to set this attribute explicitly.
Setting a valid type for a namespace but then not allowing any value
whatsoever is non-sensical.

The valid_type setter also implemented the reverse, meaning that
setting valid_type=None would automatically set dynamic=False. This
use case is a lot less common and actually leads to a lot of problems
since the default valid_type of a namespace is None. When exposing a
namespace that is dynamic, but without a particular valid_type, which
is perfectly fine logically - it just accepts any value, the exposed
namespace would lose its dynamicity. This is because the valid_type
would be set after the dynamic, resetting it to False because the
valid_type is None.

@sphuber sphuber requested review from muhrin and greschd February 12, 2020 17:55
@sphuber sphuber force-pushed the fix_135_expose_keep_dynamic_attribute branch from 1d054f1 to 5b7471d Compare February 12, 2020 18:01
@greschd
Copy link
Member

greschd commented Feb 14, 2020

Agree with the change, but I'm not sure should close #146 just yet.

As you mentioned elsewhere, the whole automatic setting of dynamic seems just a little bit too clever.. but removing it would be a breaking change.

@sphuber
Copy link
Collaborator Author

sphuber commented Feb 14, 2020

Agree with the change, but I'm not sure should close #146 just yet.

Wait you agree with the change, but we shouldn't close this PR yet? Did you really mean #146 or something else?
We can always put this in 0.15.0 since you are right that it is a breaking change

@greschd
Copy link
Member

greschd commented Feb 14, 2020

Sorry, shouldn't close #135 yet.

@greschd
Copy link
Member

greschd commented Feb 14, 2020

We can always put this in 0.15.0 since you are right that it is a breaking change

And in AiiDA I guess it can go into 1.2.0, since it is a breaking change, but only a minor one. We don't do strict semver, right?

@sphuber
Copy link
Collaborator Author

sphuber commented Feb 14, 2020

We can always put this in 0.15.0 since you are right that it is a breaking change

And in AiiDA I guess it can go into 1.2.0, since it is a breaking change, but only a minor one. We don't do strict semver, right?

We don't do strict as in no breaking changes whatsoever, zero, zilch, since that is almost impossible, but since v1.0 we do have the intention to follow it as strictly as possible and reasonable. I guess this would indeed be a case that might be acceptable since I really doubt anyone was relying on this behavior, intentional or not.

But couldn't we just follow the same logic and just put this in plumpy==0.14.6 then? The same argument holds there and in that case, it solves an existing problem for some people straight away. aiida-core==1.2.0 might be a while away.

And why shouldn't this close #135? Is this because the more refined question that you brought up later regarding precedence of dynamic value of host and donator namespace when exposing? If that is the case, would it be OK to maybe spawn off a dedicated issue for that? Since we are not yet sure what the behavior should be, it be good to close the base issue already

@greschd
Copy link
Member

greschd commented Feb 14, 2020

Well, 0.14.6 should be a "bugfix" change, no?
I'd probably have released this as 0.15, and then sort of immediately upgrade AiiDA develop. The next release would then be 1.2.0. Anyway, you're more aware of the conventions we use w.r.t. versioning, so I'll leave that up to you.

As for #135, a separate issue might indeed be cleaner.

@muhrin
Copy link
Collaborator

muhrin commented Feb 15, 2020

I'm happy witht he changes here as they stand. I'm sorry I can't comment too much on the how I think the logic should be as I'm not up to speed on the exact nature of the issue but having read #135 I'm broadly in agreement with @greschd regarding what should happen on absorb.

@sphuber
Copy link
Collaborator Author

sphuber commented Jun 5, 2020

I will merge this and release it with 0.15.0 and opened a #159 to continue the discussion

@sphuber sphuber force-pushed the fix_135_expose_keep_dynamic_attribute branch from 777ee43 to 8141488 Compare June 5, 2020 13:34
When setting `valid_type` to anything but `None` the setter will
automatically also set `dynamic` to `True`, because this almost always
what one wants and so saves having to set this attribute explicitly.
Setting a valid type for a namespace but then not allowing any value
whatsoever is non-sensical.

The `valid_type` setter also implemented the reverse, meaning that
setting `valid_type=None` would automatically set `dynamic=False`. This
use case is a lot less common and actually leads to a lot of problems
since the default `valid_type` of a namespace is `None`. When exposing a
namespace that is dynamic, but without a particular `valid_type`, which
is perfectly fine logically - it just accepts any value, the exposed
namespace would lose its dynamicity. This is because the `valid_type`
would be set after the `dynamic`, resetting it to `False` because the
`valid_type` is `None`.
@sphuber sphuber force-pushed the fix_135_expose_keep_dynamic_attribute branch from 8141488 to 2a66b31 Compare June 5, 2020 13:36
@sphuber sphuber merged commit cce7285 into develop Jun 5, 2020
@sphuber sphuber deleted the fix_135_expose_keep_dynamic_attribute branch June 5, 2020 13:38
unkcpz pushed a commit to unkcpz/plumpy that referenced this pull request Dec 14, 2024
…iidateam#146)

When setting `valid_type` to anything but `None` the setter will
automatically also set `dynamic` to `True`, because this almost always
what one wants and so saves having to set this attribute explicitly.
Setting a valid type for a namespace but then not allowing any value
whatsoever is non-sensical.

The `valid_type` setter also implemented the reverse, meaning that
setting `valid_type=None` would automatically set `dynamic=False`. This
use case is a lot less common and actually leads to a lot of problems
since the default `valid_type` of a namespace is `None`. When exposing a
namespace that is dynamic, but without a particular `valid_type`, which
is perfectly fine logically - it just accepts any value, the exposed
namespace would lose its dynamicity. This is because the `valid_type`
would be set after the `dynamic`, resetting it to `False` because the
`valid_type` is `None`.
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.

Namespaces lose "dynamic" attribute upon exposing.
3 participants