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

Deprecate signature as argument #2657

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

blythed
Copy link
Collaborator

@blythed blythed commented Nov 29, 2024

This PR disables the need to add signature='*args,**kwargs' or similar to a model.

@jieguangzhou
Copy link
Collaborator

jieguangzhou commented Dec 30, 2024

I thought about it and I’m not entirely sure if the concept of signature is really necessary for the model.

For an individual model, it actually doesn’t seem to need a signature.

The signature only comes into play when we use a trainer or a listener.

In fact, there’s a simpler logic:

For the keys used by trainer and listener:

key = "a" → "singleton" or "*args"
key = ("a", "b") → "*args"
key = {"a": "x", "b": "y"} → "**kwargs"
key = (("a", "b"), {"k": "x"}) → "*args, **kwargs"

This is a very straightforward mapping, simpler, more intuitive, and easier to maintain than using signature.

For a select and a key, we can uniquely and directly retrieve the corresponding formatted data.

WDTY?

@blythed
Copy link
Collaborator Author

blythed commented Jan 13, 2025

I thought about it and I’m not entirely sure if the concept of signature is really necessary for the model.

For an individual model, it actually doesn’t seem to need a signature.

The signature only comes into play when we use a trainer or a listener.

In fact, there’s a simpler logic:

For the keys used by trainer and listener:

key = "a" → "singleton" or "*args"
key = ("a", "b") → "*args"
key = {"a": "x", "b": "y"} → "**kwargs"
key = (("a", "b"), {"k": "x"}) → "*args, **kwargs"

This is a very straightforward mapping, simpler, more intuitive, and easier to maintain than using signature.

For a select and a key, we can uniquely and directly retrieve the corresponding formatted data.

WDTY?

Yes makes sense.

@blythed
Copy link
Collaborator Author

blythed commented Jan 14, 2025

I thought about it and I’m not entirely sure if the concept of signature is really necessary for the model.
For an individual model, it actually doesn’t seem to need a signature.
The signature only comes into play when we use a trainer or a listener.
In fact, there’s a simpler logic:
For the keys used by trainer and listener:

key = "a" → "singleton" or "*args"
key = ("a", "b") → "*args"
key = {"a": "x", "b": "y"} → "**kwargs"
key = (("a", "b"), {"k": "x"}) → "*args, **kwargs"

This is a very straightforward mapping, simpler, more intuitive, and easier to maintain than using signature.
For a select and a key, we can uniquely and directly retrieve the corresponding formatted data.
WDTY?

Yes makes sense.

After thinking about this again, the place where this is needed is in the predict_batches function, which is indeed in Model. We could potentially add this as an additional argument to predict_batches. We should also create a new function, Model.predict_many which has the same signature as predict_batches, which can be used as alternative to programming the predict_batches method.

@blythed blythed force-pushed the feature/auto-signature branch from ef827c4 to 21d4b4b Compare January 14, 2025 20:04
@blythed blythed merged commit 27a7914 into superduper-io:main Jan 14, 2025
3 checks passed
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 this pull request may close these issues.

[DEV-CON] Replace signature with auto-inferred signature (.predict) using meta-class
2 participants