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
LPS-83379 Enabled by default #61609
LPS-83379 Enabled by default #61609
Conversation
This is because tensorflow must be in the root class loader or it will crash every time the module is redeplouyed. See spotify/scio#1137 (comment)
This reverts commit 899d1fb.
…o avoid the risk of crashing portal jvm.
It will remain disabled until the whole auto tagging feature is manually enabled. This is just to be able to enable auto tagging just with one "click".
CI is automatically triggering "ci:test:sf" and "ci:test:relevant" for this pull to run Source Formatter and relevant tests. Comment "ci:test" to run the full PR Tester for this pull. |
✔️ ci:test:sf - 1 out of 1 jobs passed in 1 minute 35 seconds 486 msClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-83379-with-shuyang-changes 1 Successful Jobs:For more details click here. |
@4lejandrito can you make a follow up change to limit how many times we relaunch the tensorflow jvm? Just in case some special image could cause it crashing, and people use it to create a DDoS to us. Make the limits configurable, and once we reach that limit, just disable the tagger to prevent it from being used further. |
Hi @shuyangzhou , I'll do after @brianchandotcom merges it if that's ok. Regards |
Yeah, that's fine. Just add it to your queue and make sure it goes in before next fix pack release. Thanks! |
Cool, I'll do it tomorrow most likely. Just for your info, this is a new feature for 7.2 so it won't make it to any fix pack for the moment. |
Hmm, Brian is still auto back porting to 7.1, right? If it goes into master, it means going in 7.1 too. |
Nothing related to this new autotagging feature has been backported. @sergiogonzalez told us that he had green light from @brianchandotcom to start sending new features as long as they were in separate modules. Maybe @brianchandotcom can clarify this just in case. |
OK, that's good! Then you have plenty time to work on it. |
Merged. Thank you. View just my changes: 4lejandrito:7d1cafe184...37194b9 |
@4lejandrito I made a lot of changes in upstream. I also used a new ticket for 4e789e6 and backported it since it doesn't necessarily have anything to do with this ticket. @petershin see fa22b37 not sure why SF didn't catch that |
@brianchandotcom - There isn't a check to auto sort language keys. Will send a patch |
@4lejandrito I see this error in the upgrade console log, can you please resolve? Looks like it only affects upgrades.
cc/ @austinchiang (this is master-only. Doesn't fail test but errors are present in the log.) |
It is fixed here #61694 |
it is a dirty shutdown in the upgrade, that is still a problem, this fix is only making sure we can peacefully recover from it. |
No description provided.