Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconcile internal use of misc.as_list() with API argument types. #311

Open
eirrgang opened this issue Aug 20, 2021 · 4 comments
Open

Reconcile internal use of misc.as_list() with API argument types. #311

eirrgang opened this issue Aug 20, 2021 · 4 comments

Comments

@eirrgang
Copy link
Contributor

eirrgang commented Aug 20, 2021

as_list() is frequently used to normalize inputs for RCT commands that accept singular arguments for parameters that are generically multi-valued. However, it does not recognize non-list sequences, such as tuples or generators. This imposes unnecessary constraints and even possibly performance bottlenecks (such as when filtering requests in the raptor.Master.request_cb) or may cause unexpected behavior (if a user passes a tuple or set of states to rp.wait()).

The obvious exceptions to iterables that should be allowed are str and bytes. There are RCT objects that may be acceptable individually or in groups, but I believe they all satisfy isinstance(obj, collections.abc.Mapping). If not, maybe there is a root RCT object they are descended from?

In addition to (or instead of) updating misc.as_list(), an as_iterable() or as_rct_arg() utility function might be added and used judiciously in many of the current as_list() call sites.

Another alternative would be to add richer checking or casting based on an optional argument asserting the expected type of the list element, but that might be hard to read or surprising for RCT developers, and it would be hard to decide whether to try to convert arguments or elements or to just reject them.

@eirrgang
Copy link
Contributor Author

From @andre-merzky:

... The as_list method is used in high frequency code paths in our code, and the set of additional isinstance calls has a measurable performance impact which we don't think we can afford. I'm afraid the respective type casting has to happen on the application side (from RCT perspective).

There are several cases in raptor where it seems appropriate to allow tuples or generators to be passed instead of lists, but I do not believe these are in high frequency code paths. If as_list is going to be used under the hood without updates, we could at least try to clarify the type requirements of these signatures. Alternatively, maybe the lower-frequency code paths could use a more robust, less optimized utility than as_list?

If the type casting is necessary (are tuples and generators really not okay?), then doing it on the application side is fine, but the application programmer needs to be directed to do so more clearly.

@andre-merzky
Copy link
Member

I certainly agree on the sentiment. The API needs better and more stringent type documentation, and we can indeed reconsider the types we accept in the API. Happy to leave the issue open in that context.

@eirrgang eirrgang changed the title Let misc.as_list() have more intuitive behavior. Reconcile internal use of misc.as_list() with API argument types. Feb 21, 2022
@eirrgang
Copy link
Contributor Author

It also seems that, if as_list() is used in performance critical / high frequency code paths, it should be replaced with an is_list() to just do an assertion, and put the responsibility on the caller to provide an argument of the right type.

@andre-merzky
Copy link
Member

The calling happens in basically the same context every time, and we are confident that the current semantics is what the caller would do anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants