Skip to content

Add a convenient constructor #9707

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
Merged
merged 1 commit into from
Mar 28, 2025
Merged

Add a convenient constructor #9707

merged 1 commit into from
Mar 28, 2025

Conversation

larryliu0820
Copy link
Contributor

Summary: As titled. Use default values for topp and rng.

Differential Revision: D71956172

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Mar 27, 2025 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9707

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit b672cf3 with merge base 5531a0e (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 27, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D71956172

swolchok
swolchok reviewed Mar 27, 2025
View reviewed changes
extension/llm/runner/stats.h
long aggregate_sampling_timer_start_timestamp = 0;
};

static constexpr auto kTopp = 0.9f;
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this bc-breaking because this header doesn't include sampler.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This constant belong to sampler.h not stats.h. Anyone using this constant should already imported sampler.h so it's not breaking anything. They don't have to include stats.h right now if they just need to use this constant.

larryliu0820 added a commit that referenced this pull request Mar 27, 2025
@larryliu0820 @facebook-github-bot
Add a convenient constructor (#9707)
d79d00e 
Summary:

As titled. Use default values for `topp` and `rng`.

Reviewed By: iseeyuan

Differential Revision: D71956172
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D71956172

larryliu0820 added a commit that referenced this pull request Mar 27, 2025
@larryliu0820 @facebook-github-bot
Add a convenient constructor (#9707)
7e21fd9 
Summary:
Pull Request resolved: #9707

As titled. Use default values for `topp` and `rng`.

Reviewed By: iseeyuan

Differential Revision: D71956172
facebook-github-bot pushed a commit that referenced this pull request Mar 27, 2025
@larryliu0820 @facebook-github-bot
Add a convenient constructor (#9707)
d9e3b45 
Summary:

As titled. Use default values for `topp` and `rng`.

Reviewed By: iseeyuan

Differential Revision: D71956172
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D71956172

larryliu0820 added a commit that referenced this pull request Mar 27, 2025
@larryliu0820 @facebook-github-bot
Add a convenient constructor (#9707)
96f12fd 
Summary:

As titled. Use default values for `topp` and `rng`.

Reviewed By: iseeyuan

Differential Revision: D71956172
facebook-github-bot pushed a commit that referenced this pull request Mar 27, 2025
@larryliu0820 @facebook-github-bot
Add a convenient constructor (#9707)
3183461 
Summary:

As titled. Use default values for `topp` and `rng`.

Reviewed By: iseeyuan

Differential Revision: D71956172
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D71956172

1 similar comment
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D71956172

larryliu0820 added a commit that referenced this pull request Mar 27, 2025
@larryliu0820 @facebook-github-bot
Add a convenient constructor (#9707)
9eb1c16 
Summary:
Pull Request resolved: #9707

As titled. Use default values for `topp` and `rng`.

Reviewed By: iseeyuan

Differential Revision: D71956172
@larryliu0820 @facebook-github-bot
Add a convenient constructor (#9707)
b672cf3 
Summary:

As titled. Use default values for `topp` and `rng`.

Reviewed By: iseeyuan

Differential Revision: D71956172
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D71956172

@facebook-github-bot facebook-github-bot merged commit 8b948e8 into main Mar 28, 2025
83 checks passed
@facebook-github-bot facebook-github-bot deleted the export-D71956172 branch March 28, 2025 00:15
kirklandsign pushed a commit that referenced this pull request Apr 11, 2025
@larryliu0820 @kirklandsign
Add a convenient constructor
aa5b7be 
Differential Revision: D71956172

Pull Request resolved: #9707
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swolchok swolchok swolchok left review comments

@jackzhxng jackzhxng jackzhxng approved these changes

@iseeyuan iseeyuan Awaiting requested review from iseeyuan

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported topic: not user facing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@larryliu0820 @facebook-github-bot @swolchok @jackzhxng