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

Streamline API naming for Server* / *Callable #168

Open
th0ger opened this issue May 24, 2024 · 2 comments
Open

Streamline API naming for Server* / *Callable #168

th0ger opened this issue May 24, 2024 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@th0ger
Copy link

th0ger commented May 24, 2024

I just tried to get an overview of the server classes fingerprints (pynumaflow 0.7.0);

module class_name arg_name arg_type
pynumaflow.mapper MapServer mapper_instance MapSyncCallable
pynumaflow.mapper MapAsyncServer mapper_instance MapAsyncCallable
pynumaflow.mapper MapMultiprocServer mapper_instance MapSyncCallable
pynumaflow.reducer ReduceAsyncServer reducer_handler ReduceCallable
pynumaflow.mapstreamer MapStreamAsyncServer map_stream_instance MapStreamCallable
pynumaflow.sourcetransformer SourceTransformServer source_transform_instance SourceTransformCallable
pynumaflow.sourcetransformer SourceTransformMultiProcServer source_transform_instance SourceTransformCallable
pynumaflow.sourcer SourceServer sourcer_instance SourceCallable
pynumaflow.sourcer SourceAsyncServer sourcer_instance SourceCallable
pynumaflow.sinker SinkServer sinker_instance SyncSinkCallable
pynumaflow.sinker SinkAsyncServer sinker_instance AsyncSinkCallable
pynumaflow.sideinput SideInputServer side_input_instance RetrieverCallable
  • reducer_handler stands out, suggesting rename reducer_instance.
  • Consider dropping Sync syntax altogether, since it is verbose and will lead to inconsistent syntax over time. For example ReduceCallable needs to be renamed to ReduceSyncCallable when ReduceAsyncCallable is implemented. The Server* class names follows this convention, but the *Callables don't.
  • AsyncSinkCallable stands out to MapSyncCallable/MapAsyncCallable/MapSyncCallable in verb ordering, suggesting consitent naming SinkAsyncCallable.

Message from the maintainers:

If you wish to see this enhancement implemented please add a 👍 reaction to this issue! We often sort issues this way to know what to prioritize.

@th0ger th0ger added the enhancement New feature or request label May 24, 2024
@kohlisid
Copy link
Contributor

kohlisid commented May 24, 2024

@th0ger Thanks for pointing this out!

  1. For the reducer_handler stands out, suggesting rename reducer_instance
    --> This we can change for sure
  2. Consider dropping Sync syntax altogether,
    ---> For this we might be considering this in the next iteration during refactoring, I want to make the type checking stricter for servers which support multiple types (sync, async, etc) so that we have better handling of inconsistencies
  3. AsyncSinkCallable stands out to MapSyncCallable
    Agreed, will change this

@th0ger
Copy link
Author

th0ger commented May 24, 2024

You're welcome, thanks for considering this.
These are of course just nice-to-have breaking changes that you should wait and bundle with other API changes you plan.

@kohlisid kohlisid self-assigned this Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants