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.

Sign up for GitHub

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

Jump to bottom

Causal models only supported for text-generation task, not summarization task #972

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

Causal models only supported for text-generation task, not summarization task #972

wants to merge 11 commits into from

Conversation

njbrake
Copy link
Contributor

@njbrake njbrake commented Feb 21, 2025 •
edited
Loading

What's changing

  • Move SamplingParameters to more closely match the GenerationConfig that HF uses
  • Handle the summarization task differently from text-generation
  • Unify how we handle new tokens, and make sure that we don't allow for more tokens than the model pos emb support
  • Warn if the input exceeds the pos embeddings, but don't throw an error, truncate.

If this PR is related to an issue or closes one, please link it here.

Refs #970

How to test it

CI Tests, also load the Lumigator UI and make sure you can still test the BART CNN model.

Additional notes for reviewers

I already...

  • Tested the changes in a working environment to ensure they work as expected
  • Added some tests for any new functionality
  • Updated the documentation (both comments in code and product documentation under /docs)
  • Checked if a (backend) DB migration step was required and included it if required

@njbrake njbrake linked an issue Feb 21, 2025 that may be closed by this pull request
Unit tests using model not supported for summarization #970
Open
@github-actions github-actions bot added sdk backend frontend api Changes which impact API/presentation layer schemas Changes to schemas (which may be public facing) labels Feb 21, 2025
@njbrake njbrake marked this pull request as draft February 21, 2025 15:14
@njbrake njbrake marked this pull request as ready for review February 21, 2025 15:33
njbrake
njbrake commented Feb 21, 2025
View reviewed changes
lumigator/jobs/inference/model_clients.py Outdated
max_length = self._pipeline.model.config.max_position_embeddings
# If the model is of the HF Hub the odds of this being wrong are low, but it's still good to check that the
# tokenizer model and the model have the same max_position_embeddings
if self._pipeline.tokenizer.model_max_length != max_length:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because some models (esp older ones) Don't have a tokenizer_config file! Looking at you, facebook/bart-large-cnn lol https://huggingface.co/facebook/bart-large-cnn/discussions/71

@njbrake
Copy link
Contributor Author

njbrake commented Feb 21, 2025

It was pointed out that I accidentally remove text-generation support when merging LiteLLM. I'll put back that code #978 and then refactor this PR based on that code. Thanks!

@njbrake njbrake changed the base branch from main to 122-support-hf-clms February 21, 2025 18:09
Base automatically changed from 122-support-hf-clms to main February 21, 2025 18:41
@njbrake njbrake changed the title HF Summarization Pipeline does not support Causal Models. Causal models only supported for text-generation task, not summarization task Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpoulopoulos dpoulopoulos Awaiting requested review from dpoulopoulos

@HareeshBahuleyan HareeshBahuleyan Awaiting requested review from HareeshBahuleyan

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
api Changes which impact API/presentation layer backend frontend schemas Changes to schemas (which may be public facing) sdk
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unit tests using model not supported for summarization
1 participant
@njbrake