Skip to content

Conversation

joecummings
Copy link
Member

@joecummings joecummings commented Oct 10, 2025

Context

We previously had a custom class called SamplingConfig that would selectively map variables to vLLM's SamplingParams. While this was nice for conversion to/fro dictionary structs, it was an additional redirect for the user to understand what knobs were actually available for sampling.

Changes

  • Deleted SamplingConfig, instead relying directly on SamplingParams.
  • Added a debug log of the resolved SamplingParams
  • Updated naming in all configs, tests, etc
  • Added a link to the vLLM sampling params in every config
  • Deleted guided decoding work b/c that parameter was actually deprecated and unused outside of Judges. I think this will be formally figured out in the Judge work.

Testing

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 10, 2025
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 4.34783% with 22 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@41215d3). Learn more about missing BASE report.

Files with missing lines Patch % Lines
tests/unit_tests/test_policy_config.py 0.00% 13 Missing ⚠️
src/forge/actors/policy.py 10.00% 9 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #377   +/-   ##
=======================================
  Coverage        ?   64.36%           
=======================================
  Files           ?       79           
  Lines           ?     7728           
  Branches        ?        0           
=======================================
  Hits            ?     4974           
  Misses          ?     2754           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@joecummings joecummings marked this pull request as ready for review October 10, 2025 19:44
@joecummings joecummings force-pushed the use-native-vllm-funcs branch 2 times, most recently from aedcc27 to 39d041d Compare October 10, 2025 22:08
@Jack-Khuu
Copy link
Contributor

Oh this is a fun one, taking a look

Deleted guided decoding work b/c that parameter was actually deprecated and unused outside of Judges. I think this will be formally figured out in the Judge work.

The field is being renamed, but the underlying impl is mostly the same (i think they called it out ~2 weeks ago)

Copy link
Contributor

@Jack-Khuu Jack-Khuu left a comment

Choose a reason for hiding this comment

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

This passes the gut test

Comment on lines 39 to +40
if (prompt := cfg.get("prompt")) is None:
gd = cfg.policy.get("sampling_config", {}).get("guided_decoding", False)
prompt = "What is 3+5?" if gd else "Tell me a joke"
prompt = "Tell me a joke"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (prompt := cfg.get("prompt")) is None:
gd = cfg.policy.get("sampling_config", {}).get("guided_decoding", False)
prompt = "What is 3+5?" if gd else "Tell me a joke"
prompt = "Tell me a joke"
prompt := cfg.get("prompt", "Tell me a joke")

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh can't apply suggestion on deleted lines :/

@joecummings
Copy link
Member Author

This passes the gut test

Maslow's hierarchy of tests goes unit tests, integration tests, gut tests

@joecummings joecummings merged commit 136a5d6 into meta-pytorch:main Oct 10, 2025
6 checks passed
@joecummings joecummings deleted the use-native-vllm-funcs branch October 10, 2025 22:28
if isinstance(sampling_config, Mapping):
sampling_config = SamplingConfig(**sampling_config)
if isinstance(sampling_params, Mapping):
sampling_params = SamplingParams.from_optional(**sampling_params)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for whoever in the future needs to use rich fields of vllm/v1 in yaml:

from_optional doesn't play well with rich fields (nesting)

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.

4 participants