Closed alkuzad closed 1 month ago
dg-nvm
commented
4 months ago I created this constraint file that helps with the issue - effectivelly automatically lowers the grafana-client version to one that uses requests - https://github.com/alkuzad/python-global-constraints
amotl
commented
2 months ago Dear @alkuzad,
thanks a stack for reporting this. We certainly did not intend to cause any havocs for downstream users.
Async is nice but it was proven that it is not main contributor to the speed . Adding support for it easily is ok but not at cost of breaking other flows.
We hear you, "use the right tool for the job" is way to go. However, some people also use the library in async environments, and the solution proposed by @Ousret to provide sync & async variants appeared to be solid, so we went this way.
Niquests uses urllib3.future library that replaces urllib3 library even if you installed both, so installing two will conflict. Grafana-client is a library often used by DevOps who also use boto3 or other libraries together connecting different systems and platforms together. Adding a dependency for library that eradicates proper urllib3 and introduces new bugs that breaks environment is problematic. We can no longer add grafana-client into our common toolset and we have to extract it to another tool or use pre 4.0 version.
Ouch. We certainly did not foresee this drawback. Can you think of any solution for this problem, @Ousret?
With kind regards, Andreas.
chintal
commented
2 months ago Shadowing urllib3 feels like it would be a problematic approach to take. We might wish to reconsider whether niquests + urllib3.future is really the way to go.
Aside from the serious security and stability issues it might introduce into production environments, I would have expected urllib3.future to be entirely backwards compatible with urllib3. i.e., change of the version from 2.2.2 to 2.8.902 should not have broken anything (other than audits going a little nuts). If urllib3.future guaranteed this, anything that breaks would be a urllib3.future bug and the issue would be best addressed by urllib3.future. I don't think urllib3.future provides any such guarantee, though, given they say:
urllib3.future goes beyond supported features while remaining mostly compatible.
urllib3.future's decision to replace urllib3 while not ensuring API compatibility moves the problem to a significantly more dangerous place. Since urllib3 is very widely used, this effectively plants landmines all across the python environment it is installed in. In the context of production environments, this would (correctly) be considered entirely unacceptable.
This leaves us with very few options that I can see if we wish to continue using niquests+urllib3.future. The ideal solution would be to try to see what's breaking for the OP and fix urllib3.future so it does not. However, we are too far removed from that library and the OP's problem to realistically be able to do so, and we don't know how many other such issues might arise from other users. The only viable option I can think of is to try to vendor niquests and urllib3.future, similar to what pip does.
Ousret
commented
2 months ago Hello there,
I will try to be as complete as possible, so that you may take a firmer grasp on what is going on there.
Origin story:
The user (OP) did came to urllib3-future and complain about an issue he would not disclose https://github.com/jawah/urllib3.future/issues/133 on Jul 12. I did offer to fix the situation within hours, but he refused for a reason that is his own.
Then on Aug 14, someone else opened graciously a PR that helped find the issue he most likely encountered https://github.com/jawah/urllib3.future/pull/142
The OP more likely intended to enforce his vision by ultimatums.
Async is nice but it was proven that it is not main contributor to the speed . Adding support for it easily is ok but not at cost of breaking other flows.
It's not true anymore. I speak from experience, async > threading in Python. Period. That may change with the GIL-free interpreter soon.
@amotl
Ouch. We certainly did not foresee this drawback. Can you think of any solution for this problem, @Ousret?
Absolutely, it was fixed in https://github.com/jawah/urllib3.future/pull/142
@chintal
By reading your analysis, you are mostly wrong on several critical point.
Shadowing urllib3 feels like it would be a problematic approach to take.
Not at all. We invite you to carefully look at how we work at urllib3-future and how we ensure it's not so problematic.
Aside from the serious security and stability issues it might introduce into production environments
There's no evidence to substantiate that claim. urllib3-future did not reinvent the wheel, and every bit of security, bugfixes, tests (amongst other things) are still there. urllib3 is not perfect either, and thinking issues only appear with tinier solution is not the way to go.
In a full year, we had six valid reports on urllib3-future, or if you prefer 1 report every two months. And the tendency only goes down so far. urllib3 has more than ten valid bugfixes in the same period.
I would have expected urllib3.future to be entirely backwards compatible with urllib3. i.e., change of the version from 2.2.2 to 2.8.902 should not have broken anything
++
Since urllib3 is very widely used, this effectively plants landmines all across the python environment it is installed in. In the context of production environments, this would (correctly) be considered entirely unacceptable.
It is fully backward compatible. See https://github.com/jawah/urllib3.future/actions And, urllib3 2.0 is not backward compatible with 1.x, and 2.1 not with 2.0, also 2.2 not with 2.1, there's tiny breaking changes.
The only viable option I can think of is to try to vendor niquests and urllib3.future, similar to what pip does.
It's most certainly "a solution" but not a great one. It implies that the burden will increase here. How long until we can dispatch a fix from Niquests to vendor and release? How will you vendor sub dependencies?
@amotl @chintal
Unfortunately, at the time being, we cannot do better than this. For reasons explained in https://github.com/jawah/urllib3.future?tab=readme-ov-file#notes--frequently-asked-questions and https://niquests.readthedocs.io/en/latest/community/faq.html#what-is-urllib3-future and https://github.com/jawah/urllib3.future/issues/91
Then, I would argue that:
Finally,
If this would come out of control, meaning the errors and bad surprises would increase and irremediably prove that this solution can't work, then I will personally see the matter resolved any way I can.
No issues emerged since the last incident. I am fairly convinced that the issues will remain to a minimum.
If @amotl decide to not trust Niquests, and urllib3-future I will take responsibility and actions. I kept my word, and I intend to continue doing so.
urllib3-future deserve a chance to prove itself. We are long gone from the era where IE was imposed, and Microsoft boldly claimed that it was better for users for their own security. Alternative must exist.
Regards,
chintal
commented
2 months ago @Ousret Thank you for the information. I was not aware of the OP's report to you and what it said.
I also apologize for the tone of my original comment. It was in no way intended to be derogatory to you or to the library.
Firstly, let's get this out of the way. The OP's claim that async produces no performance benefits is patently absurd. There is certainly a benefit to async, and not just for the sake of performance. I simply decided to ignore that part of his rant, because arguing the point on such statements is usually a lost cause.
Secondly, yes, urllib3-future and every library deserves a chance to prove itself. It is certainly encouraging to me as a user of grafana-client that even though urllib3 is being poked, you're around to help iron out any issues that might arise. I should say that I have grafana client in a python environment with a large collection of other packages, many of which I expect would be using urllib3, and I did not even realize urllib-future was shadowing urllib3 until now. This is certainly impressive in terms of the package's compatibility with urllib3.
That said, shadowing is a bigger problem than you think. As long as grafana-client is installed by itself or with a small collection of packages, what you're doing is perfectly fine. When it gets pulled into an environment with a number of other components, however, is when it becomes a cause for concern. This has nothing to do with the quality of your code. In controlled environments, code is validated against a stable, well defined environment. If the environment is to change, that's fine, but it needs to be an explicit choice. There always has been and always will be code relying on weird corner cases and undocumented behavior.
If I validate component A - which depends on urllib3 - against urllib3, and then install component A into an environment which also needs to run component B which has pulled in urllib3-future, then all of a sudden my code in component A is running against urllib3-future. This is not something I've validated, and it is, in fact, contrary to the specifications I lay down when I list my dependencies for component A. Even a glance at pip freeze will not tell me that I'm actually using urllib3-future, because urllib3 remains installed (Correct me if I'm wrong here. I expect this is the case because my requirements remain satisfied). I need to already know that urllib3-future shadows urllib3, to identify that this is happening.
Yes, urllib3 breaks compatibility with urllib2. But urllib3 doesn't shadow urllib2. Compatibility breaking between urllib 2.1 and urllib 2.2 etc. is also easily controlled by the well understood and common practice of pinning specific versions in production. A practice, I might point out, which shadowing bypasses. I may not even know that I need to pin urllib3-future, because as far as I know grafana-client is dealing with versioning that package and is the only code that cares what version it is.
I do understand why shadowing urllib3 was necessary for the functionality you want to provide. You could, in principle, have achieved similar functionality by calling it urllib3_future, and having requests etc. add hooks or config to be able to switch the underlying implementation. That's difficult because it's going to be incredibly difficult to get code into requests. The reason it's difficult is not dissimilar from the reason why shadowing urllib3 can be problematic. Changes to widely used software, even with the best of intentions, have a way of triggering unexpected side effects.
In any case, grafana-client has chosen niquests. The original bug report, combined with the OP's report to urllib3.future, does not warrant a change to that decision, in my opinion.
I apologize again for my abrasive tone. I do not wish to accuse, offend, or belittle.
Ousret
commented
2 months ago No "offense" taken in that matter. We know how people tend to "react" on urllib3-future, and it often lacks knowledge.
If I validate component A - which depends on urllib3 - against urllib3, and then install component A into an environment which also needs to run component B which has pulled in urllib3-future [...]
We know the solution isn't perfect and we already stated it. Their is some place for debate for sure. We did our best, to date.
You could, in principle, have achieved similar functionality by calling it urllib3_future, and having requests etc. add hooks or config to be able to switch the underlying implementation. That's difficult [...]
Not just difficult, but almost close to impossible, untestable, etc... If someone clever could find the perfect workaround, we'll be listening for sure.
If everything is OK, I am letting someone else closing this issue.
Regards,
amotl
commented
1 month ago Hi. Thank you for your elaborate conversation. I figure everything is reasonably in order. Therefore, I would be inclined to close this issue, but would like to quickly come back to the original report:
Niquests uses
urllib3.futurelibrary that replacesurllib3library even if you installed both, so installing two will conflict.
So, is there an actual conflict or problem?
@Ousret: You referred to https://github.com/jawah/urllib3.future/pull/142, that resolves a specific problem. Shall we introduce some dependency constraint that will use the subsequent release as the minimum version for urllib3.future?
Apologies that I didn't follow the complete conversation, I just wanted to ask about any pragmatic advises that could be taken on grafana-client, if any, before actually closing the issue.
Ousret
commented
1 month ago So, is there an actual conflict or problem?
None, if we speak on the "issue" side. The shadowing thing will remain until a proper solution exist.
Shall we introduce some dependency constraint that will use the subsequent release as the minimum version for urllib3.future?
No need, Niquests does already pin it to a higher version. It would bring unnecessary complexity to grafana-client.
regards,
amotl
commented
1 month ago Thank you. Closing now.
Describe the bug
Niquests uses urllib3.future library that replaces urllib3 library even if you installed both, so installing two will conflict.
You either guarantee that grafana-client works or boto3 works but not in the same time.
To Reproduce Steps to reproduce the behavior:
rm -rf test && python3 -mvenv test && . ./test/bin/activate && pip install boto3 1>/dev/null && cat test/lib/python3.12/site-packages/urllib3/_version.py && pip install grafana-client 1> /dev/null && cat test/lib/python3.12/site-packages/urllib3/_version.pyExpected behavior
I would see both times:
Instead of it changing to this on second print:
Versions
grafana-client: [ grafana-client==4.1.0 ]Additional context
Grafana-client is a library often used by DevOps who also use boto3 or other libraries together connecting different systems and platforms together. Adding a dependency for library that eradicates proper urllib3 and introduces new bugs that breaks environment is problematic. We can no longer add grafana-client into our common toolset and we have to extract it to another tool or use pre 4.0 version.
Async is nice but it was proven that it is not main contributor to the speed . Adding support for it easily is ok but not at cost of breaking other flows.