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

There is no such thing in just traitlets as Sentinel #28

Closed
mprogram opened this issue Jul 13, 2018 · 10 comments · Fixed by #29
Closed

There is no such thing in just traitlets as Sentinel #28

mprogram opened this issue Jul 13, 2018 · 10 comments · Fixed by #29

Comments

@mprogram
Copy link

…it actually was/is in traitlets.utils.sentinel for a long time being, so the correct curated syntax, taken from line 4 would be (forgive my ignorance, but how 0.2.x releases were being made, so it has slipped through?):
from traitlets.utils.sentinel import Sentinel

@SylvainCorlay
Copy link
Member

I am not sure that we need to do that since Sentinel is imported at the top level in traitlets.

Is there a reason why it would be cleaner to import it from utils?

@mprogram
Copy link
Author

err… Sentinel is not being imported at the top level in traitlets, had you tried to run the line?:
>>> from traitlets import TraitType, TraitError, Undefined, Sentinel
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ImportError: cannot import name 'Sentinel'

@vidartf
Copy link
Member

vidartf commented Jul 18, 2018

Maybe this is inconsistent between versions of traitlets (it definitely works for some versions). @mprogram which version of traitlets are you using?

@mprogram
Copy link
Author

@vidartf, in traittypes setup.py requires traitlets>=4.2.2 – in turn, in traitlets line from .sentinel import Sentinel was changed to line from .utils.sentinel import Sentinel by this commit in early 2015, while the releases from 4.0.0 through 4.3.2 has happened to be from 11.07.2015 through 23.02.2017, the last commit being just 9 days ago (not counting numerous pull requests).

@SylvainCorlay
Copy link
Member

err… Sentinel is not being imported at the top level in traitlets, had you tried to run the line?:

from traitlets import TraitType, TraitError, Undefined, Sentinel
Traceback (most recent call last):
File "", line 1, in
ImportError: cannot import name 'Sentinel'

@mprogram from traitlets import TraitType, TraitError, Undefined, Sentinel appears to work.

Do you know a version of traitlets for which it would not

@mprogram
Copy link
Author

mprogram commented Jul 18, 2018

@SylvainCorlay, with all due respect, shall it be a repeating question, to which I've already posted the answer? To recount, and based on the actual absence of class Sentinel in just traitlets in the relevant part of the traitlets code (so, it could not just work), it is the master branch of traitlets, as well as the 4.0.0 through 4.3.2 releases from 11.07.2015 through 23.02.2017 (admittedly, I have not tested all of them), where it has moved, in early 2015, to utils/sentinel.py, at the time when setup.py of traittypes requires traitlets to be >=4.2.2.

It appeared to me as such a simple issue, I'm truly surprised I was not understood from my very first comment. It's truly discouraging, and re-typing the whole comment only cloaks the clarity of original reporting. Am I missing, might be, some possibly hidden backgrounds on reporting issues on GitHub?…

EDITS: …happened to be the self-contained proof of the last paragraph

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Jul 18, 2018

From a technical standpoint

I have tested that all released versions of traitlets support from traitlets import Sentinel.

screen shot 2018-07-18 at 12 59 20 pm

screen shot 2018-07-18 at 1 00 17 pm

screen shot 2018-07-18 at 1 00 37 pm

screen shot 2018-07-18 at 1 00 59 pm

screen shot 2018-07-18 at 1 01 42 pm

There appears to be a regression in the development version of traitlets in that regard.
EDIT: this was a deliberate change by @minrk to not make Sentinel and other internal utilities directly importable: ipython/traitlets#416

Traittypes is a dependency to a number of projects (bqplot, ipydatawidgets, ipyvolume), so I was surprised it would be so "obviously" broken, hence the question about the version that you used.

Now regarding your last message

The developers maintaining Project Jupyter are volunteers, and take the time to respond to your issue in their free time. We are all giving each other a service here. I get that working with software is frustrating and discouraging at times, but I would really like that you remain civil in your interactions with the volunteers maintaining the project.

@vidartf
Copy link
Member

vidartf commented Jul 18, 2018

@SylvainCorlay Thanks for doing the legwork here and actually testing the different versions! So the issue here is that Sentinel is no longer exported on the top-level in the development version of traitlets. We should probably copy out the definition from traitlets then in order to future-proof ourselves.

@SylvainCorlay
Copy link
Member

Sounds good to me!

@mprogram
Copy link
Author

mprogram commented Jul 18, 2018

@SylvainCorlay, my apologies for not testing the released versions of traitlets – it was more difficult for me to do that as I'm struggling to maintain a tiny single system-wide set of the bleeding-edge python packages for my personal project on a weak computer system. That known to me the fact of traittypes being a dependency to a number of major projects must have triggered a reasonable doubt the issue might have been with the development version of traitlets is my faux pas. It happens in written communications one's intentions when read perceived differently, that were with my edits, to "self-contained" which I referred to: I was, indeed, so frustrated with rewriting my explanation, correcting typos, that had to edit it four times. However, I watched my language, and at no time implied anything personally wrong, or behaved in uncivil manner, I'm truly sorry if it might have been perceived uncollaborative.

The reason to write a closing comment after the issue is over, however, is different from a technical point of view:

I think, on more general, if not yet philosophical level, the decision to import … Sentinel at the top level directly from traitlets while it was in .utils.sentinel was conceived prematurely, – as much anticipated avoidance of the use of 'internal utilities such as class of' in the above referenced pull #416 was a time bomb since the first release of traitlets in 2015. This was likewise evidenced by a much heated discussion about moving pip's 10 "guts" to pip._internal. Looking forward-retrospectively, I suggest reverting the closing merge, while thanking @vidartf for the pull, and replace it with my suggestion from the first comment (ignoring the rest of the discussion). The reason for it is that possible changes to traitlets.utils.sentinel would not be automatically accounted for in future versions of traittypes, and the move would maintain a good backward compatibility referring to the internal utility of traitlets' class Sentinel in "an internal, non-top level way".

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 a pull request may close this issue.

3 participants