Use Session for HTTP requests #738

hokieg3n1us · 2021-08-02T01:33:07Z

This proposed change would resolve any issues similar to issue #17 .

Description: Include a dictionary of configuration for a requests Session that is instantiated by the downloader, including keys for auth, cert, and verify. This allows a user to modify that session by providing a tuple of username and password for basic authentication, or provide client and server certificates. It should also improve performance when making repeated requests to the same host by re-using the underlying urllib3 connection.

Example Usage:

import osmnx as ox

session_config = {}
session_config['auth'] = ('username', 'password')
session_config['cert'] = '/path/to/cert'
session_config['verify'] = '/path/to/CA/'

osmnx.config(session_config=session_config)

Include a default requests.Session as part of settings that is used for HTTP requests to OSM APIs, and can be overridden using the utils.config. This should improve performance by re-using the underlying TCP connection when sending multiple requests to the same host, and allows a user of osmnx to override the session to provide client/server certificates when making request to any OSM API that uses 2-way SSL verification.

Correct formatting for imports.

Update downloader.py to fix out-of-scope variable.

Re-format code using black (for line length)

…/osmnx into use-requests-session

Switched to Ubuntu dev machine to properly run lint_test.sh, after introducing formatting issue through Github merge conflict interface.

codecov · 2021-08-11T17:49:24Z

Codecov Report

Merging #738 (3c35302) into main (81db14d) will decrease coverage by 0.12%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main     #738      +/-   ##
==========================================
- Coverage   96.77%   96.65%   -0.13%     
==========================================
  Files          24       24              
  Lines        2422     2423       +1     
==========================================
- Hits         2344     2342       -2     
- Misses         78       81       +3

Impacted Files	Coverage Δ
osmnx/downloader.py	`95.26% <86.36%> (-1.44%)`	⬇️
osmnx/settings.py	`100.00% <100.00%> (ø)`
osmnx/utils.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81db14d...3c35302. Read the comment docs.

gboeing · 2021-08-11T18:00:12Z

Thanks @hokieg3n1us let me think about this. Two questions for you:

Would requests.Session have any conflicts/redundancies with the new explicit IP address resolution fixes released in v1.1.1 (Resolve Overpass API requests to a specific IP address #699)?
Could you add more detail about why/how a user would use this in the docstring?

Updated docstring to better describe how to use session. Since Session creates a persistent TCP connection that is used when requests are sent to the same host/port, remove redundant fix from #699. Unable to duplicate network error from tests after running multiple times on Python 3.6, 3.7, 3.8, and 3.9.

hokieg3n1us · 2021-08-12T02:47:54Z

@gboeing

You're correct that it should be redundant. The requests.Session will create a persistent urllib3 connection that will be re-used for requests to the same host/port, so I backed out the _config_dns function.
I updated the docstring. Maybe we should link to Requests documentation for Advanced Usage to describe the customization that can be done on the session.

Diagnosing the intermittent test failures, determined that the root cause was the server closing the connection due to inactivity. Refactored the settings to expose a dictionary of attributes that can be configured for a requests.Session that is instantiated when a call is made to _osm_network_download, _osm_geometries_download, or nominatum_request. Less elegant, but should allow users to provide authentication and 2-way SSL configuration, while also improving performance during repeated calls to the overpass API.

hokieg3n1us · 2021-08-12T03:57:07Z

@gboeing

I diagnosed the intermittent failures (inactivity on the underlying TCP connection causing the server to close), and refactored significantly to still gain the benefits of using a requests Session, while providing some of the more important configuration options to a user. More of the underlying Session parameters can be exposed as necessary.

Properly check if key exists in session_config, so a user can only provide keys as necessary for their environment.

moving files used for local testing of different python versions

gboeing

I've left just a few comments. My main overall question would be: have you tested this to confirm consistent IP address resolution in line with what #699 provided? If so, could you document those test methods/results here? Thanks!

gboeing · 2021-08-12T16:24:58Z

osmnx/downloader.py

+            utils.log(f"Pausing {this_pause} seconds before making HTTP POST request")
+            time.sleep(this_pause)


These need to be indented back out of the if block.

That produces an out of scope variable reference to this_pause similar to the recently fixed issue for the response status code.

gboeing · 2021-08-12T16:26:47Z

osmnx/utils.py

+        and verify as keys. auth is a tuple of (username, password). cert is the
+        path to a single file (containing the private key and certificate) or a tuple
+        of both files' paths. verify can be the path to a CA_BUNDLE file, or directory
+        of trusted CAs.


It might be easier and more future-proof to just point the user toward the requests documentation on requests.Session.auth, cert, and path since we're just passing these values through to those requests.Session attributes.

gboeing · 2021-08-12T16:29:28Z

osmnx/downloader.py

@@ -275,7 +278,7 @@ def _get_pause(base_endpoint, recursive_delay=5, default_duration=60):

    try:
        url = base_endpoint.rstrip("/") + "/status"
-        response = settings.session.get(url, headers=_get_http_headers())
+        response = session.get(url, headers=_get_http_headers())


_create_session is not called in the _get_pause function, but it is called in the other functions that make HTTP requests to the Overpass server?

gboeing · 2021-08-12T16:30:34Z

osmnx/downloader.py

@@ -451,6 +454,8 @@ def _osm_network_download(polygon, network_type, custom_filter):
    else:
        osm_filter = _get_osm_filter(network_type)

+    session = _create_session()


I notice that _create_session is called 3 times (i.e. separately in the different "download" functions). Is there a reason to do it this way, rather than creating a session one time, for example whenever ox.config is called, and then just using that session throughout?

hokieg3n1us · 2021-08-12T17:59:19Z

I am closing this pull request because I cannot reproduce the intermittent issues causing the remote server to close the connection, and that's difficult to debug properly without more information about OSM's service layer. It's also difficult to elegantly construct a session, without refactoring the downloader.py to be instantiated classes that live/die during graph construction since you can't create a single Session to be used for the entirety of osmnx, because the client can't hold open an inactive TCP connection.

I plan to identify a route of cleanly passing cert, auth, and verify directly to the requests.post/requests.get.

gboeing · 2021-08-12T21:21:55Z

New PR for development at #745.

hokieg3n1us and others added 7 commits August 1, 2021 21:25

Update settings.py

aa69248

Correct formatting for imports.

Resolve merge conflict

7584155

Update downloader.py to fix out-of-scope variable.

Merge branch 'main' into use-requests-session

a4f05fe

Format code using black

e2235c8

Re-format code using black (for line length)

Merge branch 'use-requests-session' of https://github.com/hokieg3n1us…

63559e1

…/osmnx into use-requests-session

Corrected linting issue.

afe903b

Switched to Ubuntu dev machine to properly run lint_test.sh, after introducing formatting issue through Github merge conflict interface.

hokieg3n1us added 2 commits August 12, 2021 01:22

Check key exists in session_config

de82017

Properly check if key exists in session_config, so a user can only provide keys as necessary for their environment.

cleaning up extraneous files

3c35302

moving files used for local testing of different python versions

gboeing requested changes Aug 12, 2021

View reviewed changes

hokieg3n1us closed this Aug 12, 2021

hokieg3n1us deleted the use-requests-session branch August 12, 2021 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Session for HTTP requests #738

Use Session for HTTP requests #738

hokieg3n1us commented Aug 2, 2021 •

edited

Loading

codecov bot commented Aug 11, 2021 •

edited

Loading

gboeing commented Aug 11, 2021

hokieg3n1us commented Aug 12, 2021

hokieg3n1us commented Aug 12, 2021

gboeing left a comment

gboeing Aug 12, 2021 •

edited

Loading

hokieg3n1us Aug 12, 2021 •

edited

Loading

gboeing Aug 12, 2021

gboeing Aug 12, 2021

gboeing Aug 12, 2021

hokieg3n1us commented Aug 12, 2021

gboeing commented Aug 12, 2021

		utils.log(f"Pausing {this_pause} seconds before making HTTP POST request")
		time.sleep(this_pause)

Use Session for HTTP requests #738

Use Session for HTTP requests #738

Conversation

hokieg3n1us commented Aug 2, 2021 • edited Loading

codecov bot commented Aug 11, 2021 • edited Loading

Codecov Report

gboeing commented Aug 11, 2021

hokieg3n1us commented Aug 12, 2021

hokieg3n1us commented Aug 12, 2021

gboeing left a comment

Choose a reason for hiding this comment

gboeing Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

hokieg3n1us Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

gboeing Aug 12, 2021

Choose a reason for hiding this comment

gboeing Aug 12, 2021

Choose a reason for hiding this comment

gboeing Aug 12, 2021

Choose a reason for hiding this comment

hokieg3n1us commented Aug 12, 2021

gboeing commented Aug 12, 2021

hokieg3n1us commented Aug 2, 2021 •

edited

Loading

codecov bot commented Aug 11, 2021 •

edited

Loading

gboeing Aug 12, 2021 •

edited

Loading

hokieg3n1us Aug 12, 2021 •

edited

Loading