Defined namespace and implementation on Parameters object #230
Conversation
|
How does the deprecate decorator currently work? At least for the time being, I'd want such warnings not to be visible unless a particular user (ourselves, for the meantime) specifically enabled them via a module variable or maybe an rc file. It will take some time for us to get all our own codebases up to date, and such warnings shouldn't be on by default until we at least have our own house in order. |
I totally agree with all that. I'm currently prototyping how we can issue deprecation warnings later on and I'm not actually proposing that we actually issue such warnings just yet. We can keep the decorators and simply drop the line that generates the warning once we decide how exactly we want to proceed. |
|
I'm totally in favor of this kind of change, and look forward to reviewing this PR (tomorrow). I did try to look at the diff on github now, but it's a bit confusing. I either need to inspect it with git locally or remind myself what is in param ;)
I like your constant faith in green tests meaning everything is ok ;) We have a general problem with warnings (not specific to this PR or even param). First, developers locally don't usually see/notice/report warnings (something which in theory should be improved by using pyctdev). Second, travis does not flag warnings. In this PR, there appear to be thousands of warnings :) E.g. see https://travis-ci.org/ioam/param/jobs/391336086#L453 onwards. Presumably most are from tests that need updating or something. I don't mean these need to be addressed now before any review/progress, I just wanted to mention it as something that will need to be addressed at some point. |
param/parameterized.py
Outdated
| if isinstance(val,Parameter): | ||
| paramdict[name] = val | ||
| settings = ['%s=%s' % (name,repr(val)) | ||
| for name,val in self.get_param_values()] |
There was a problem hiding this comment.
I'm struggling with the diff here, so maybe I'm confused...but won't this kind of thing trigger the deprecation warning?
There was a problem hiding this comment.
Yes. That is one of the things I would like to discuss soon.
Sounds like we all agree about that. Also, my gut says I probably wouldn't want to tell people to switch their code to a new thing straight away. As in, it would be great to get this kind of change in and start using it ourselves, and not feel inhibited from making further changes to it if we want after using it a bit. If we've just told people they need to update their code because the old way is deprecated and will disappear, we might then feel bad about making further changes to it straight away. (Maybe, not sure. Just thinking about how it would be nice to make some changes without worrying about existing code/users until later. You could imagine going further, and e.g. not even making new API visible unless deliberately turned on, but I don't think that's necessary...) |
I would be in favor of this if we wanted to do a general API cleanup on the What I do know is I want to use a cleaned up API soon and eventually have parameterized objects that don't have their API mixed up with param related stuff (i.e drop the compatibility stuff). Lastly, I should mention that to me a clean API is a small API with a limited number of general methods that are powerful enough for you to do what you want. I would want some of the existing cruft to go away rather than just being replaced. |
param/parameterized.py
Outdated
| @classmethod | ||
| @Parameters.deprecate | ||
| def _add_parameter(cls, param_name,param_obj): | ||
| cls.param._add_parameter(param_name,param_obj) |
There was a problem hiding this comment.
Should use return. Note that in principle this is not public and does not need to be mirrored.
|
I have found that I can transfer the docstrings to the stubs using: try:
# Fails for __db_print due to name mangling.
# Could fall back to something fancier (e.g mangled name in another try)
inner.__doc__ = getattr(cls, fn.__name__).__doc__
except:
passThat said, I think my earlier approach might be better as it directs people to the param sub object which is what we want people to be using: inner.__doc__= "Inspect .param.%s method for the full docstring" % fn.__name__ |
|
In c1d7b37 I try to filter the output of
I don't want to make things too complicated so I'm leaving this `dir`` problem alone for now. @jbednar @philippjfr any suggestions? How important is it that dir reports a clean namespace..even if it isn't true? |
|
I guess you could filter the mangled names one by one? My goal with dir() would be for it to report our new (evolving) API only -- hiding any old, deprecated, now incorrect ways of using things. It should allow the new, good, appropriate ways of using things to be explored easily, with no distracting cruft. |
|
In 963211f I tried to move as much as possible to use the |
Most of the mangled names can be filtered nicely enough though I still have trouble figuring out how to implement |
|
Jean-Luc pointed out privately that script_repr and pprint are problematic because they are designed to be overridden by subclasses, including ParameterizedFunction within param itself, but also potentially user-defined subclasses that want to control the representation of those objects. That's a good point, and one that I hadn't fully appreciated until now. I think I vote for keeping script_repr and pprint as methods on Parameterized itself, to avoid breaking this behavior; we could hack it for our own purposes, but in general subclasses do need to extend this functionality, and I don't think we should be making doing so require miracles. I think we should also look whether there any other methods where we anticipate that a subclass may need to override the behavior, and ensure that the implementation of that method remains on Parameterized and not on the new subobject. Note that we don't necessarily need to continue making those methods public; we could still rename the method to make it single-underscore private, to provide inheritability and overriding per subclass; the user interface can still be via the Parameters subobject to have a cleaner API and namespace. |
I agree with this approach for now but one day, I think we ought to have a release that breaks backwards compatibility. My own feeling is that |
|
I'm not talking about compatibility here, but functionality. Specifically, if we have functionality that can primarily be implemented in the base class but may need to be overridden in the subclass, that functionality ought to be implemented in the class itself so that no magic is required to get things to respect the class hierarchy. script_repr and pprint are just two examples; state saving and serialization are others. Whether to continue offering script_repr officially or hiding it is a separate question that should be debated elsewhere. |
|
@jbednar @philippjfr @ceball I am done with all the main changes I want to make for this PR. The only thing left to do is to fix the tests that I had to comment out: there is probably somewhere I need to quality with I don't expect that fixing those tests should result in any fundamental change to what I've already done here so I think this PR is ready for you to look at. |
| @@ -208,7 +208,7 @@ def _rational(self, val): | |||
| elif hasattr(val, 'numer'): | |||
| (numer, denom) = (int(val.numer()), int(val.denom())) | |||
| else: | |||
| param.main.warning("Casting type '%s' to Fraction.fraction" | |||
| param.main.param.warning("Casting type '%s' to Fraction.fraction" | |||
There was a problem hiding this comment.
Are .warning, .message, etc. candidates for leaving on the main Parameterized namespace? Just asking; I don't have a strong opinion either way. The argument for not having them on .param is just that they aren't in any way about Parameters; they are about the Parameterized object itself (it's meant to be the object that is generating the message, not the param subobject, unless the message was about Parameters).
There was a problem hiding this comment.
I have mixed feelings about this as I am not sure why logging is related to the core of what param offers. I feel many projects will just use the python logging module their own way so I think I would vote against putting them on the parameterized object and maybe consider deprecating these logging features entirely someday.
There was a problem hiding this comment.
Perhaps instead of being methods, the messaging could be extracted into a separate function, taking the Parameterized as an argument? That way the existence of this functionality wouldn't affect anything not using it, but it would still be there as a uniform way to use Parameterized objects together with the logging module.
In any case, if the methods will end up being deprecated, then they presumably should not move onto the .param object; there is little point in moving things and then deleting them. Deprecated things should presumably stay in Parameterized and get deprecated there.
Note that param itself uses .warning(), but at least one of those and possibly both are candidates for being exceptions rather than warnings.
There was a problem hiding this comment.
I think this discussion can continue in #233 and doesn't need to be decided just yet.
param/parameterized.py
Outdated
| Initializes all the Parameters by looking up appropriate | ||
| default values (see __param_inheritance()) and setting | ||
| attrib_names (see _set_names()). | ||
| _disable_stubs = False |
There was a problem hiding this comment.
Addressed in e32ecfb
param/parameterized.py
Outdated
|
|
||
| abstract = property(__is_abstract) | ||
| inner.__doc__= "Inspect .param.%s method for the full docstring" % fn.__name__ | ||
| return inner |
There was a problem hiding this comment.
Presumably this will break HoloViews tests that show the output of hv.help, until hv.help is updated? Seems important to be able to have a version of param that allows us to update HoloViews to the new API without having things broken in the meantime.
There was a problem hiding this comment.
Good point. I'll make sure hv.help keeps working which should be fine as most of that logic is in param.ipython.
There was a problem hiding this comment.
I've checked and the output of hv.help is unaffected.
param/parameterized.py
Outdated
| separate line). | ||
|
|
||
| NOTE: pprint will replace script_repr in a future version of | ||
| param, but is not yet a complete replacement for script_repr. |
There was a problem hiding this comment.
Anybody know what is lacking in pprint for it to replace script_repr?
There was a problem hiding this comment.
I don't know I'm afraid. @ceball ?
param/parameterized.py
Outdated
| prefix="" | ||
| # CEBALERT: remove with script_repr | ||
| elif hasattr(val,'script_repr'): | ||
| rep=val.script_repr(imports, prefix+" ") |
There was a problem hiding this comment.
@ceball, is this the bit of pprint that still requires .script_repr()? Seems odd that a .script_repr() method would take precedence over .pprint() if .pprint() is meant to replace .script_repr().
| @classmethod | ||
| def params(cls,parameter_name=None): | ||
| Returns 'classname(parameter1=x,parameter2=y,...)', listing | ||
| all the parameters of this object. |
There was a problem hiding this comment.
What's the difference between repr and script_repr? I remember there was one at some time, but can no longer remember why we needed script_repr to be distinct.
There was a problem hiding this comment.
Was it just the indentation and newlines? If so maybe that could be enabled in some other way, leaving a top-level script_repr() function but no .script_repr() methods?
There was a problem hiding this comment.
... leaving a top-level script_repr() function but no .script_repr() methods?
I don't know the details of whether this is possible, but I do think it is desirable...
|
One comment I made for myself is that there are a bunch of scary |
|
I've been testing this with holoviews nosetests. I had to upgrade pandas to get rid of a bunch of test error/failures and there is still one failing test with the released version of param. After my latest two commits, I get the same failure with this PR. |
|
I also double checked |
|
In the end, I did manage to move |
|
This all seems great to me. @jlstevens, how would you feel if I made a PR against this one to deal with these issues:
Apart from that, we should merge. (Then I propose to make a dev release straight away, so we learn about things that have gone wrong as soon as possible...) |
|
Just to note that I think the outcome of the We could revisit this decision if seeing the old names is annoying enough (presumably whatever we want is possible - it's just a question of the effort required...). |
From the sound of it, those other changes would be quite small in which case a PR on top of this one is fine. These all sound like reasonable changes but if you think you'll come up with more changes or find that these tweaks are non-trivial, I would prefer to see this PR merged and then you could make a separate PR on top of master. Your summary of the situation around |
|
Seems like Chris's changes should go into this PR, since they involve not
removing some functionality.
…On Mon, Jun 25, 2018, 06:26 Jean-Luc Stevens ***@***.***> wrote:
... how would you feel if I made a PR against this one to deal with these
issues
From the sound of it, those other changes would be quite small in which
case a PR on top of this one is fine. These all sound like reasonable
changes but if you think you'll come up with more changes or find that
these tweaks are non-trivial, I would prefer to see this PR merged and then
you could make a separate PR on top of master.
Your summary of the situation around __dir__ is accurate though I would
also mention is that one of the trickier parts that would be required to
get this working is getting dir to work properly at the class level.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#230 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABnfCBxAVIzeTJzzfe-7CX7F6dpZ0ga7ks5uAMjSgaJpZM4UkclW>
.
|
|
@ceball Now your PR is merged, it is your turn to decide whether this PR is ready! |
This PR is still WIP. There are a lot of lines changed from the very first commit as this was the sort of task I couldn't tell would be viable until it was all done.
In short it moves most of the code of
Parameterizedonto aparamobjects of typeParameterswhich also acts as a namespace. The goal is to make this the face of the public API cleaning up the namespace of classes subclassed by users. Here is an example for abothmethodparams:HOW IT WORKS
Parametersis instantiated by the metaclass or the regular constructor.self_as the first argument.cls = self_.clsafter which the code is unchanged.self_or_cls = self_.cls if self_.self is None else self_.selfafter which the code is unchanged.self = self_.selfafter which the code is unchanged.Notes:
paramobject is per instance when necessary allowing it to hold state either at the class or instance level: this will be important for recording information regarding parameter dependencies etc.@as_uninitializedare used in theParameterizedconstructor and I have left them on that class.Parameterized(e.g__repr__)_instantiate_paramwhich I can move as it used__dict__and is used from theParameterizedconstructor via_setup_paramsParametersand accessed throughself_to avoid Python name mangling issues. This came up for__db_printin particular.self_or_cls,cls_or_self,cls_or_slforslf_or_cls? I have seen at least three of these...TODO
ParameterizedFunction? [Yes except for.instance].paramnamespace so we can just delete the aliases when the time comes.paramobject to migrate users to the namespace object.@as_uninitializedand special methods too? [Yes]