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

Allow batching for feature-extraction #106

Merged
merged 6 commits into from
Jun 16, 2021
Merged

Allow batching for feature-extraction #106

merged 6 commits into from
Jun 16, 2021

Conversation

osanseviero
Copy link
Contributor

No description provided.

Copy link
Contributor

@Narsil Narsil left a comment

Choose a reason for hiding this comment

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

I think it's ok in the current form, but I would suggest to use the already existing validation tools we have if we can (so we don't need to introduce BATCHED_ENABLED_PIPELINES.

@osanseviero
Copy link
Contributor Author

One last question for me is in check_inputs. I see three options with this change

  • Strictest: raise an error if the tag is not found in INPUTS_MAPPING.
  • Current: raise an error if inputs is not a string
  • Flexible: Just allow any input as we do with params.

I would go with the strictest option unless you have objections

@osanseviero osanseviero requested a review from Narsil June 15, 2021 15:13
Copy link
Contributor

@Narsil Narsil left a comment

Choose a reason for hiding this comment

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

LGTM

@Narsil
Copy link
Contributor

Narsil commented Jun 16, 2021

Strictest seems ok, but would need to add the rest of the pipelines if we're missing some.

Also seems better IMO to be as strict as possible (as long as errors messages are understandable)

@osanseviero osanseviero merged commit 9aa9b20 into main Jun 16, 2021
@osanseviero osanseviero deleted the batch_fe branch June 16, 2021 14:32
@osanseviero
Copy link
Contributor Author

FYI @Kvit and @Matthieu-Tinycoaching, I think you were interested in this :) we'll update the sentence-transformers docker image after next release.

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