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

Add with_split to DatasetDict.map #7368

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jp1924
Copy link

@jp1924 jp1924 commented Jan 13, 2025

@jp1924
Copy link
Author

jp1924 commented Jan 13, 2025

Can you check this out, @lhoestq?

@jp1924
Copy link
Author

jp1924 commented Jan 20, 2025

cc @lhoestq @albertvillanova

@jp1924
Copy link
Author

jp1924 commented Feb 4, 2025

@lhoestq

@jp1924
Copy link
Author

jp1924 commented Feb 12, 2025

@lhoestq

1 similar comment
@jp1924
Copy link
Author

jp1924 commented Feb 13, 2025

@lhoestq

Comment on lines 905 to 906
if with_split and "split" in fn_kwargs:
raise ValueError("The key 'split' is reserved for the split name and cannot be passed in fn_kwargs.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to avoid this issue we use positional arguments for indices/rank to avoid this issue (this way users can name their parameters arbitrarily), maybe we can use this trick: https://stackoverflow.com/a/66274908 ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lhoestq
Ah, I see what you mean.

So what you're saying is that there may be people who were already using fn_kwagrs to do splits, and my implementation may cause conflicts? I agree.

So let's use partial instead of fn_kwargs to solve the problem? Sounds like a good idea!

So I redefined partial and made it like this, can you take a look?

class bind(partial):
    def __call__(self, *fn_args, **fn_kwargs):
        return self.func(*fn_args, *self.args, **fn_kwargs)

@jp1924 jp1924 requested a review from lhoestq February 21, 2025 07:50
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.

2 participants