#957 New multicall interface
Merged 2 years ago by mikem. Opened 3 years ago by mikem.

No commits found

The original multicall interface is less than optimal:

  • It is modal. Once session.multicall is set, the session cannot be used normally
  • Confusing to have both session.multicall and session.multiCall()
  • Processing the return value is tedious

These changes create a new implementation outside of ClientSession. The old way will still work.

With this new implementation:

  • a multicall is tracked as an instance of MultiCallSession
  • the original session is unaffected
  • multiple multicalls can be managed in parallel, if desired
  • MultiCallSession behaves more or less like a session in multicall mode
  • method calls return a VirtualCall instance that can later be used to access the result
  • MultiCallSession can be used as a context manager, ensuring that the calls are executed

Here is one example of possible use:

with session.multicall(strict=True) as m:
    for package in to_add:
        m.packageListAdd(tag, package, options.owner, **opts)

Here is an example that make use of the VirtualCall values:

tinfos = {}
with session.multicall(strict=True) as m:
    for task_id in task_ids:
        tinfos[task_id] = m.getTaskInfo(task_id)
for task_id, tinfo in tinfos.items():
    tinfo = tinfo.result

About the multicall attribute....

We already had both multicall and multiCall. I was hesitant to add yet another similar sounding attribute to the ClientSession class to access this functionality. Luckily, it was possible to redefine the multicall attribute to serve both as a settable boolean (original behavior) and a callable (new behavior).

This seems to work well, but I'd appreciate any feedback.

I still need to add some docs and additional unit tests for this.

Also, I should add a batch option for parity with #900

should MultiCallNotReady inherit GenericError?

Nice, I like the context manager approach.

Does it make sense to store arguments in VirtualCall as original args/kwargs for some possible introspection? I know, that it is really simple to recompose them from params, but it could be more human-friendly interface (e.g defining str for it printing original call). Also, isn't the right place for encode_args inside format as it is the only place where it matters?

should MultiCallNotReady inherit GenericError?

Not really, because this is client-only code. GenericError and its children are for exceptions that the hub will return as faults.

Granted, I'm not super happy with Koji's exception hierarchy...

Does it make sense to store arguments in VirtualCall as original args/kwargs for some possible introspection?

Yeah, I was pondering that. I'll adjust that when I get a chance

2 new commits added

  • cleanup
  • store args/kwargs in VirtualCall instances
3 years ago

1 new commit added

  • stub unit test
3 years ago

1 new commit added

  • provide multicall attribute for backwards compat
3 years ago

rebased onto a0d5971791552ec2bf1ce5c07d5f20d49fd3dccd

3 years ago

Rebased with updates:

  • expanded unit test a bit
  • added batch option for parity with #900
  • used the new interface to add multicall to a few more cli commands

Still need to document the new interface, but I'd like to merge #1358 first since that also contains some multicall docs (that will need updating after this).

rebased onto 54da20e

2 years ago

rebased with docs updates

@tkopecek @julian8628 I think this is about ready. Appreciate any feedback.

:thumbsup:

Question, maybe for 1.19 - would we like to replace old implementation with special instance of MultiCallSession? There is a lot of duplicated code and if we ensure, that all calls when session.multicall is true are caught by this special instance, it should be less code/maintenance.

1 new commit added

  • fix typo
2 years ago

Question, maybe for 1.19 - would we like to replace old implementation with special instance of MultiCallSession?

Sure. I was initially hesitant to touch the original code, but once this has seen a real release it should be fine.

Also it is an important change, so there should be an issue linked before merging (Just to see it in roadmap, etc.)

Commit d612eac fixes this pull-request

Pull-Request has been merged by mikem

2 years ago

there should be an issue linked

Ah good point. I'll add one.