Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.9%. Comparing base (d553546) to head (adad63e).
Report is 196 commits behind head on main.

@@           Coverage Diff           @@
##            main   #1183     +/-   ##
=======================================
+ Coverage   81.7%   81.9%   +0.2%     
=======================================
  Files         54      61      +7     
  Lines       4208    4750    +542     
  Branches     889    1015    +126     
=======================================
+ Hits        3439    3892    +453     
- Misses       574     625     +51     
- Partials     195     233     +38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

by default not enabled to keep the current behavior as-is

Just out of curiosity - is there any reason not to enable it by default, i.e. is it not a strict enhancement to go from request.get to self.session.get? What could break? Looks like a straight upgrade from docs and stack overflow as request.get creates a one-time-use Session anyway.

Thinking about client usage of WorkflowsService with a Session, I think it might be better to implement context management for WorkflowsService for basic Session usage:

with WorkflowsService(...) as ws:
    ws.create(...)
    ws.list(...)

This would implicitly use a single session. If we want to also allow users to pass in the session for flexibility as per @alicederyn's comment, then they can follow a similar pattern.

with request.Session(...) as session:
    ws = WorkflowsService(session=session, ...)
    ws.create(...)
    ws.list(...)

It seems reusing a "closed" session just reopens it, and you can close multiple times, so using a "dead" session doesn't seem to be a problem:

>>> import requests
>>> session = requests.Session()
>>> session.get("https://example.com/")
<Response [200]>
>>> session.close()
>>> session.get("https://example.com/")
<Response [200]>
>>> session.close()
>>> session.close()

I was unable to find a clear statement that sessions were thread-safe in requests. I did find a closed issue asking for it to be documented one way or another but I couldn't tell what the close status was intended to be. I'm also not sure if the service classes are intended to be thread-safe themselves?

Just out of curiosity - is there any reason not to enable it by default, i.e. is it not a strict enhancement to go from request.get to self.session.get?

Oh, I fully agree :) and I think its fits anyway the "spirit" of having a WorkflowsService: it makes sense for it to hold a session to the Argo Server for the duration of its existence. I just initially thought of introducing this change as opt-in, but if we all agree then it can be the default.

Note that the Workflow class creates a default WorkflowsService if not provided, so this would also implicitly initialize and keep one session alive for the duration of the Workflow object (i.e. keep connections opened if calls were made) - and this would also not call it as context manager. So maybe the default service for workflows shouldn't use a session?

Thinking about client usage of WorkflowsService with a Session, I think it might be better to implement context management for WorkflowsService for basic Session usage:

I can add a .close() method to the Service which explicitly calls the .close() of the Session, and then yes it's just convenient to expose the context manager interface too.

This would implicitly use a single session. If we want to also allow users to pass in the session for flexibility as per @alicederyn's comment, then they can follow a similar pattern.

Perhaps it would be better to let the WorkflowService open and close the user provided Session, rather than relying on the user to open/close it. Also because we need to do it anyway if the user doesn't pass a custom Session and we use the regular one.

session = MyCustomSession()
with WorkflowsService(session=session) as ws:
    # will open/close the session
    ws.list(...)
    with Workflow(..., workflows_service=ws) as wf1:
        ...
    with Workflow(..., workflows_service=ws) as wf2:
        ...
    with Workflow(...) as wf3:
        # wf3.workflows_service != ws
        ...

It is probably better to still have to explicitly pass the service to any Workflow created under the context manager block ( Explicit is better than implicit.)

I was unable to find a clear statement that sessions were thread-safe in requests.

It's a long-standing question indeed, I've always sided with the "better safe than sorry" approach and used one session per thread. But since the urllib3 pool is thread safe in the newer versions (2022+), it is probably fine

I'm also not sure if the service classes are intended to be thread-safe themselves?

I feel like it should be thread safe: If you have tens of Workflows to submit at once, it makes sense to use one service and reuse connections. Else you just end up with the current behavior of opening one session thus one connection per call.