Use requests.Session object to reuse connections in PolicyClient

[MattiasDC]

Why are these changes needed?

Uses connections/resources more efficiently in PolicyClient. By using a requests.Session object, instead of using requests directly, connections are reused.

Related issue number

Closes #30136

Checks

• I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
• I've run scripts/format.sh to lint the changes in this PR.
• [] I've included any doc changes needed for https://docs.ray.io/en/master/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

• If you'd like to keep this open, just leave any comment, and the stale label will be removed.

[MattiasDC]

@sven1977 this has been marked as stale, could someone do a review?

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

• If you'd like to keep this open, just leave any comment, and the stale label will be removed.

[MattiasDC]

@sven1977 this has been marked as stale, could someone do a review?

[ArturNiederfahrenhorst]

@sven1977 @kouroshHakha According to https://requests.readthedocs.io/en/latest/user/advanced/#session-objects, sessions can bring significant speedups.

I've timed this a little and from a quick look I could not observe a speedup.
Nevertheless, since the code definitely works, I think we should merge this.

[kouroshHakha]

I wonder if keeping the session open can cause any problems / leaks. Usually it has to be closed. Nevertheless the PR makes sense. @MattiasDC if you were to do session.close() where should that happen?

[MattiasDC]

Hi @kouroshHakha,

I agree that it's cleaner to close the session. Typically a session is automatically closed when you use it as a context manager. There is an example in the requests doc.
Using it here as a context manager would imply that either the session object has to be passed as an argument to the constructor of the PolicyClient, and the context manager is created 'outside' of the PolicyClient, or we make PolicyClient a context manager itself.

Alternatively we simply call self.session.close() in the destructor of the PolicyClient. This seems to me the approach with the least impact on the user, while still adhering to closing sessions. If you're ok with this approach, I can update the PR.

[kouroshHakha]

@MattiasDC yep that's what I thought about too. How about we make an optional arg for policyClient called session that allows users to pass in their pre constructed session from outside. In this case, the caller will be responsible for closing the session. However if the user does not provide the session we resort to the old method?

[MattiasDC]

@kouroshHakha I agree with your suggestion and pushed it

[ArturNiederfahrenhorst]

Cool stuff! :)

1. Since the tests are running on nightly wheels, they failed because we are migrating major stuff there (@MattiasDC please push again tomorrow, CI should run on updated wheels then)
2. One last minor request: Can you document the reasoning in the code? Basically, include in the docstring that using a session can lead to speedups and that it must be closed by the creator of the session itself.

Thanks for this PR!

[MattiasDC]

@ArturNiederfahrenhorst can you have a look at the ones still failing and let me know if they are caused by my PR?

[kouroshHakha]

All RLlib tests are passing. Merging now ...