Add support for urllib3 v2 #719

iherasymenko · 2024-04-16T18:55:57Z

Description

Allows using urllib3 v2 if Python version is >= 3.10. The same technique is applied in https://github.com/boto/botocore/pull/3141/files.

Issues Resolved

#628

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

saimedhi · 2024-04-16T19:45:41Z

Hello @iherasymenko, thank you for your contribution. Could you kindly include a few pertinent tests, please?

dblock · 2024-04-16T21:14:44Z

Maybe we can introduce an integration test that uses each version of the lib?

SamStephens · 2024-04-17T21:51:30Z

setup.py

@@ -54,7 +54,8 @@
    if package == MODULE_DIR or package.startswith(MODULE_DIR + ".")
 ]
 install_requires = [
-    "urllib3>=1.26.18, <2",
+    'urllib3>=1.26.18,<1.27 ; python_version < "3.10"',


Why <1.27 rather than <2? Surely we want those who cannot use urllib3 2.x to at least get updates to 1.x?

This also mirrors what botocore did. I believe the original rationale was that 1.27 may introduce some breaking changes.

SamStephens · 2024-04-17T21:51:56Z

setup.py

@@ -54,7 +54,8 @@
    if package == MODULE_DIR or package.startswith(MODULE_DIR + ".")
 ]
 install_requires = [
-    "urllib3>=1.26.18, <2",
+    'urllib3>=1.26.18,<1.27 ; python_version < "3.10"',
+    'urllib3>=1.26.18,!=2.2.0,<3 ; python_version >= "3.10"',


Why the specific exclusion of 2.2.0?

This mirrors what botocore did. 2.2.0 has some breaking changes.

boto/botocore#3141

boto/botocore#3138 (comment)

james-certn · 2024-04-17T22:20:33Z

Naive and hardline thinking: why preserve backward urllib3 compatibility in opensearchpy at all? If someone is tied to an old version of urllib3, they can use an older version of opensearchpy.

I get the desire for flexibility and supporting the most users possible, giving the latest to all users - but at some point the overhead isn't worth it, surely.

That aside, how would one best test this PR's changes in opensearchpy?

dblock · 2024-04-17T22:37:42Z

Naive and hardline thinking: why preserve backward urllib3 compatibility in opensearchpy at all? If someone is tied to an old version of urllib3, they can use an older version of opensearchpy.

I get the desire for flexibility and supporting the most users possible, giving the latest to all users - but at some point the overhead isn't worth it, surely.

That's all there is to it, flexibility. Generally if something is possible we tend not to break it to reduce downstream pain. If there's a good reason not to, or it's too hard to maintain, then we can drop it.

That aside, how would one best test this PR's changes in opensearchpy?

Since we're explicitly saying that we support multiple incompatible dependencies I think we need to introduce some way of doing integration tests where one vs. another version of the library is loaded. I don't know how to do this in Python best, in my Ruby experience we make separate Gemfile's in subfolders and run rspec against those (example).

If this is hard to do, I would be open to merging this as is. Let's get your questions answered on the compatible versions, I think those make a lot of sense.

iherasymenko · 2024-04-18T01:12:10Z

@james-certn The opensearch-py library is frequently (if not always) used together with the AWS Python SDK (https://github.com/boto/botocore/). Preferably, this library stays compatible with the Python 3.9 Lambda deployments, i.e. we don't pin the version of urllib3 to >2.x (as the urllib3 version there is pinned to 1.x) and, at the same time, we don't leave the version of urllib3 unpinned (>1.x) as it caused #628.

@dblock @saimedhi I'm thinking about adding a separate Integration Tests job that uses the public.ecr.aws/lambda/python:3.9 container so that we can make sure in a Python 3.9 environment, the right version gets picked up and this version does not conflict with the bundled boto3/botocore AWS Python SDK. Would this be a reasonable testing approach?

dblock · 2024-04-18T12:45:53Z

@dblock @saimedhi I'm thinking about adding a separate Integration Tests job that uses the public.ecr.aws/lambda/python:3.9 container so that we can make sure in a Python 3.9 environment, the right version gets picked up and this version does not conflict with the bundled boto3/botocore AWS Python SDK. Would this be a reasonable testing approach?

I would start simpler and definitely not attempt to actually make AWS calls (we want to do that, but setting up infrastructure that runs permanently is currently difficult, we need credits, etc.). Given a certain configuration (e.g. in a Pipfile), you end up with a specific version of the dependent library, hard-coded in a Python test.

james-certn · 2024-04-19T22:22:02Z

I'm not sure that I've seen tests in projects for this kind of requirements configuration. Not that I'm against the idea, but it does seem (subjectively) irregular. Maybe I just don't get out enough. :)

dblock

I am good with merging this (please update CHANGELOG, see below) and open an issue for tests if you aren't going to attempt it.

dblock · 2024-04-22T12:50:44Z

CHANGELOG.md

@@ -3,6 +3,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)

 ## [Unreleased]
 ### Added
+- Added support for urllib3 2+ in Python 3.10+


Add the PR number here like the other lines, please?

Signed-off-by: Ihor Herasymenko <ihor.herasymenko@ada.support>

iherasymenko · 2024-04-22T14:39:24Z

@dblock Thanks. I updated the PR. I'll try to allocate some time to work on the tests this week.

dblock

Let's merge since this is the same as in botocore.

james-certn · 2024-04-23T17:25:44Z

Sorry for the Github noise, but when would a release around this be planned? Right now, the 2.5.0 versioned package as is does block a subset of users from leveraging the latest and greatest.

A ballpark of a day, a week, a month - that's all I'm asking for. ;) Thank you!

saimedhi · 2024-04-23T18:07:52Z

Sorry for the Github noise, but when would a release around this be planned? Right now, the 2.5.0 versioned package as is does block a subset of users from leveraging the latest and greatest.

A ballpark of a day, a week, a month - that's all I'm asking for. ;) Thank you!

Hello @james-certn, could you please create an issue requesting a 2.6.0 release? Similar to issues #561 and #695. Thank you!

Signed-off-by: Ihor Herasymenko <ihor.herasymenko@ada.support>

iherasymenko requested review from VachaShah, dblock, harshavamsi, axeoman, Shephalimittal, saimedhi and florianvazelle as code owners April 16, 2024 18:55

dblock mentioned this pull request Apr 17, 2024

[FEATURE] Add support for urllib3 > v2 #628

Closed

SamStephens reviewed Apr 17, 2024

View reviewed changes

saimedhi added 2.0 and removed 2.0 labels Apr 18, 2024

dblock previously approved these changes Apr 22, 2024

View reviewed changes

Add support for urllib3 v2

0ede397

Signed-off-by: Ihor Herasymenko <ihor.herasymenko@ada.support>

iherasymenko dismissed dblock’s stale review via 0ede397 April 22, 2024 14:37

dblock approved these changes Apr 22, 2024

View reviewed changes

dblock merged commit 54ad04d into opensearch-project:main Apr 22, 2024
54 checks passed

iherasymenko deleted the urllib3-v2-support branch April 22, 2024 22:47

james-certn mentioned this pull request Apr 23, 2024

[NEW RELEASE] Request to prepare & release 2.5.1 #733

Closed

saimedhi added the backport 2.5 label May 23, 2024

dblock pushed a commit to dblock/opensearch-py that referenced this pull request Aug 15, 2024

Add support for urllib3 v2 (opensearch-project#719)

2443d6b

Signed-off-by: Ihor Herasymenko <ihor.herasymenko@ada.support>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for urllib3 v2 #719

Add support for urllib3 v2 #719

iherasymenko commented Apr 16, 2024

saimedhi commented Apr 16, 2024

dblock commented Apr 16, 2024

SamStephens Apr 17, 2024

iherasymenko Apr 18, 2024

SamStephens Apr 17, 2024

iherasymenko Apr 18, 2024 •

edited

Loading

james-certn commented Apr 17, 2024

dblock commented Apr 17, 2024 •

edited

Loading

iherasymenko commented Apr 18, 2024 •

edited

Loading

dblock commented Apr 18, 2024

james-certn commented Apr 19, 2024

dblock left a comment

dblock Apr 22, 2024

iherasymenko Apr 22, 2024

iherasymenko commented Apr 22, 2024

dblock left a comment

james-certn commented Apr 23, 2024

saimedhi commented Apr 23, 2024

Add support for urllib3 v2 #719

Add support for urllib3 v2 #719

Conversation

iherasymenko commented Apr 16, 2024

Description

Issues Resolved

saimedhi commented Apr 16, 2024

dblock commented Apr 16, 2024

SamStephens Apr 17, 2024

Choose a reason for hiding this comment

iherasymenko Apr 18, 2024

Choose a reason for hiding this comment

SamStephens Apr 17, 2024

Choose a reason for hiding this comment

iherasymenko Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

james-certn commented Apr 17, 2024

dblock commented Apr 17, 2024 • edited Loading

iherasymenko commented Apr 18, 2024 • edited Loading

dblock commented Apr 18, 2024

james-certn commented Apr 19, 2024

dblock left a comment

Choose a reason for hiding this comment

dblock Apr 22, 2024

Choose a reason for hiding this comment

iherasymenko Apr 22, 2024

Choose a reason for hiding this comment

iherasymenko commented Apr 22, 2024

dblock left a comment

Choose a reason for hiding this comment

james-certn commented Apr 23, 2024

saimedhi commented Apr 23, 2024

iherasymenko Apr 18, 2024 •

edited

Loading

dblock commented Apr 17, 2024 •

edited

Loading

iherasymenko commented Apr 18, 2024 •

edited

Loading