-
Notifications
You must be signed in to change notification settings - Fork 101
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
Is ipywidgets really required? #219
Comments
Hey @PeterJCLaw , thanks for noticing this! I will review those 4 dependencies today and see if I can remove them. Less is more, as they say! |
Here's my notes:
|
Thanks for looking at this. In general I would strongly discourage any mention of transient dependencies within a library's requirements. (Obviously it's fine to include transients which are also directly used by the library). Including them tends to create confusion and forces a particular setup upon all users, opening everyone up to potential security issues even if there's a short-term benefit for some subset of them. Ideally a library's requirements should specify exactly and only the packages which the library requires and use version ranges to specify those it is compatible with. The version of transient dependencies should have no impact on the library itself and should thus not be specified. This ensures that end-users have the clearest idea of which packages it depends upon and have the most freedom to setup and/or secure their environment. For 1) are you running that in a Jupyter notebook or similar? What happens if you run the equivalent code (can you share it?) as a plain command line script? This sounds like there's an optional dependency here which I appreciate that security concerns have motivated the pinning, however those security issues will always be the responsibility of the end-user to address. The responsibility of a library with regards to security issues in dependencies exists only as far as a) direct dependencies only and then b) ensuring compatibility with patched versions & supporting those versions in the permitted ranges. Libraries doing otherwise actually make dependency management considerably harder for end-users as it becomes unclear what the true dependencies are, which (as alluded to above) potentially creates other security issues. Consider what would happen if a newer version of |
Hey @PeterJCLaw , i totally agree with your approach here Let me see if you agree with the following proposed changes:
Thoughts? |
Yup sounds good. A quick test of I've used Up to you how you want the extras of your package to work. |
Sweet. Thanks for the extra input and validation on this, I'll put up a PR soon and add you as a reviewer |
@jmcarpenter2 Thanks for your quick response. From experience, I believe ipy* packages such as Regarding Looking back, if what's suggested here is correct, then maybe you wanted |
Thanks @xquyvu that's a good callout. I will definitely remove I am seeing strange behavior with At first, I was thinking... "this is fine, it gives a warning to |
For review: #220 |
Maybe just I would discourage anything which only mentions the progress bar unless
Thanks, that looks like it's moving in the right direction. There's a lot going on in that PR though. I would tend to encourage keeping PRs to a single topic so that they're easier to review, easier to revert if needed and avoids one set of changes being held up by discussion in an unrelated set of changes. Just like with commits, the rule of thumb of "smallest useful increment" works well for PRs too. |
Thanks for the input, I think I agree. I should've submitted separate PRs for each issue and then done a release PR. |
#220 is merged, thanks all for input! |
Hi,
This package looks like it could offer our data scientists a substantial speedup, so in that regard it looks great. However it appears to pull in a number of packages which seem to be unused, together these packages significantly increase the requirements footprint of this otherwise small package. Notably this dependency ends up pulling in quite a lot of packages that we'd rather not deploy into a production system, meaning that we're less likely to be able to use it.
Is
ipywidgets
really needed? Could it be removed?It looks like the dependency appeared as a result of #42, though I'm not really sure what the underlying issue was there. I'm not massively familiar with Jupyter notebooks so I don't know whether there's an implicit dependency that that creates?
Other dependencies which appear unused:
bleach
cloudpickle
parso
The text was updated successfully, but these errors were encountered: