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

Convert SamplingParams.strategy to a union #767

Merged
merged 4 commits into from
Jan 15, 2025
Merged

Convert SamplingParams.strategy to a union #767

merged 4 commits into from
Jan 15, 2025

Conversation

hardikjshah
Copy link
Contributor

@hardikjshah hardikjshah commented Jan 15, 2025

What does this PR do?

Cleans up how we provide sampling params. Earlier, strategy was an enum and all params (top_p, temperature, top_k) across all strategies were grouped. We now have a strategy union object with each strategy (greedy, top_p, top_k) having its corresponding params.
Earlier,

class SamplingParams: 
    strategy: enum ()
    top_p, temperature, top_k and other params

However, the strategy field was not being used in any providers making it confusing to know the exact sampling behavior purely based on the params since you could pass temperature, top_p, top_k and how the provider would interpret those would not be clear.

Hence we introduced -- a union where the strategy and relevant params are all clubbed together to avoid this confusion.

Have updated all providers, tests, notebooks, readme and otehr places where sampling params was being used to use the new format.

Test Plan

pytest llama_stack/providers/tests/inference/groq/test_groq_utils.py
// inference on ollama, fireworks and together
with-proxy pytest -v -s -k "ollama" --inference-model="meta-llama/Llama-3.1-8B-Instruct" llama_stack/providers/tests/inference/test_text_inference.py
// agents on fireworks
pytest -v -s -k 'fireworks and create_agent' --inference-model="meta-llama/Llama-3.1-8B-Instruct" llama_stack/providers/tests/agents/test_agents.py --safety-shield="meta-llama/Llama-Guard-3-8B"

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Ran pre-commit to handle lint / formatting issues.
  • Read the contributor guideline,
    Pull Request section?
  • Updated relevant documentation.
  • Wrote necessary unit or integration tests.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jan 15, 2025
@hardikjshah hardikjshah changed the title Clean up SamplingParams Strategy to be a union to avoid grouping params across all strategies Convert SamplingParams.strategy to a union Jan 15, 2025
Copy link
Contributor

@ashwinb ashwinb left a comment

Choose a reason for hiding this comment

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

looks good to me! thank you for going through all the providers....

@ashwinb ashwinb merged commit a51c8b4 into main Jan 15, 2025
2 checks passed
@ashwinb ashwinb deleted the sampling branch January 15, 2025 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants