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

Remove the custom sampling loop (SequenceGenerator) #1449

Merged
merged 1 commit into from
Feb 26, 2025

Conversation

RobinPicard
Copy link
Contributor

@RobinPicard RobinPicard commented Feb 25, 2025

Addresses issue #1422

The objective of this PR is to remove the custom sampling loop (and related entities) that are not used anymore or can be replaced with other elements that are already in outlines. We want to make this commit such that it's convenient to revert the change proposed in the future.

What is modified:

  • Remove SequenceGenerator (it was already not used anymore)
  • Remove SequenceGeneratorAdapter
  • Modify the generate.<something> functions to use Generator instead of SequenceGeneratorAdapter
  • Remove the generate tests
  • Remove the samplers module and associated tests/documentation (not needed anymore as it was used for providing a sampler for the custom loop)

What is not modified:

  • The VLLM and ExLlamaV2Model models that have not been refactored yet and thus still expect to interact with SequenceGeneratorAdapter
  • The cookbooks

@RobinPicard RobinPicard added enhancement structured generation Linked to structured generation labels Feb 25, 2025
@RobinPicard RobinPicard self-assigned this Feb 25, 2025
@RobinPicard RobinPicard linked an issue Feb 25, 2025 that may be closed by this pull request
@RobinPicard RobinPicard requested a review from rlouf February 25, 2025 15:51
@RobinPicard RobinPicard added this to the 1.0 milestone Feb 25, 2025
@rlouf
Copy link
Member

rlouf commented Feb 25, 2025

I see there's a lot mixed in the last commit, but could we keep SequenceGeneratorAdapter for now and delete the rest?

@RobinPicard
Copy link
Contributor Author

If we remove Sampler too we would still need to modify many files.

@rlouf
Copy link
Member

rlouf commented Feb 25, 2025

Let's keep them for the moment then, I don't want the branch to be in a broken state even if temporary.

@RobinPicard
Copy link
Contributor Author

I modified the commit to only remove SequenceGenerator. I created a separate issue for the rest of: #1450. It should be quite straightforward after we have finished refactoring all models and have updated/deprecated the generate.<something> functions.

@rlouf rlouf merged commit c812d16 into dottxt-ai:v1.0 Feb 26, 2025
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement structured generation Linked to structured generation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the custom sampling loop
2 participants