Skip to content

Conversation

YuqingJ
Copy link
Contributor

@YuqingJ YuqingJ commented Apr 23, 2024

Stack from ghstack (oldest at bottom):

Test: The added unit test fails before this fix. But it passes now after the fix.

The fix is coming from @swolchok in D56087067.

Copy link

pytorch-bot bot commented Apr 23, 2024

🔗 Helpful Links

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

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit 45af315 with merge base c82fcb7 (image):

FLAKY - The following job failed but was likely due to flakiness present on trunk:

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

@pytorch-bot pytorch-bot bot added the release notes: jit release notes category label Apr 23, 2024
YuqingJ added a commit that referenced this pull request Apr 23, 2024
Copy link
Contributor

@davidberard98 davidberard98 left a comment

Choose a reason for hiding this comment

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

sweet, thanks (to you and @swolchok)!

note: I was wondering whether it's worth putting this behind a JK. I feel like probably not:

  • we can't train with static runtime, so there shouldn't be any models trained with this bad behavior
  • it seems basically impossible that you'd have a model that's (a) trained on the correct behavior (b) now serving with incorrect static runtime behavior, and (c) would start behaving more badly once we fix the static runtime behavior.

@YuqingJ
Copy link
Contributor Author

YuqingJ commented Apr 23, 2024

sweet, thanks (to you and @swolchok)!

note: I was wondering whether it's worth putting this behind a JK. I feel like probably not:

  • we can't train with static runtime, so there shouldn't be any models trained with this bad behavior
  • it seems basically impossible that you'd have a model that's (a) trained on the correct behavior (b) now serving with incorrect static runtime behavior, and (c) would start behaving more badly once we fix the static runtime behavior.

@houseroad Do you think this is a risky change to the existing models?

@YuqingJ
Copy link
Contributor Author

YuqingJ commented Apr 24, 2024

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Apr 24, 2024
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

alat-rights pushed a commit to alat-rights/pytorch that referenced this pull request Apr 26, 2024
…h#124789)

Test: The added unit test fails before this fix. But it passes now after the fix.

The fix is coming from @swolchok in D56087067.

Pull Request resolved: pytorch#124789
Approved by: https://github.com/davidberard98
petrex pushed a commit to petrex/pytorch that referenced this pull request May 3, 2024
…h#124789)

Test: The added unit test fails before this fix. But it passes now after the fix.

The fix is coming from @swolchok in D56087067.

Pull Request resolved: pytorch#124789
Approved by: https://github.com/davidberard98
@github-actions github-actions bot deleted the gh/yuqingj/1/head branch June 2, 2024 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: jit release notes category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants