Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[core][state][no_early_kickoff] Add "humanify" feature to StateSchema #35059
[core][state][no_early_kickoff] Add "humanify" feature to StateSchema #35059
Changes from 18 commits
24158f2
b323ff8
6dd01e2
135d804
310e0ab
676d4e9
a2284e5
86f67ab
f8f82a5
b3c5488
0b8c88e
f7b25fb
d21cba6
a05f225
e93dd82
226bde0
049ea07
f810bbb
21c3290
31246b9
75a06f6
1b678b4
54cd9a5
0c4b566
3c5a2ee
8ae3ff9
92269b9
7a185fc
8b8eb35
b629102
f58d757
ff592dc
3ba5046
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let' also add a try...except around this and logger errors instead of failing:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i found out an issue which raised error due to some of the data not correct, I think let's do best effort here instead of disallowing users to see the outputs.
#35130
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the try-except.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just confirming, the issue doesn't have anything to do with this PR? I tried running without the Humanify logic, but still get the incorrect data (year 584556019 for worker_launch_time)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am new to this;
By "best effort," what did you have in mind? If there is an error in the format_fn, we just leave the data as is. Where do we disallow users to see the outputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, added a null check to pass state_log unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's not caused by this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, thanks for reviewing! I got 20/20 for my school's open source assessment :)