Skip to content
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

[logging] Don't try to kill autoscaler during log monitor cleanup #14261

Merged
merged 1 commit into from
Feb 23, 2021

Conversation

FarzanT
Copy link
Contributor

@FarzanT FarzanT commented Feb 22, 2021

Why are these changes needed?

If the worker_pid is the string autoscale, the os.kill function will through a type error as it only accepts type int.

Related issue number

Closes #12565

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@wuisawesome wuisawesome self-assigned this Feb 23, 2021
@wuisawesome
Copy link
Contributor

Thanks for the contribution! Will merge once the tests (presumably) pass.

@wuisawesome wuisawesome changed the title Ignore worker_pid == "autoscale" when closing file descriptors [logging] Don't try to kill autoscaler during log monitor cleanup Feb 23, 2021
@rkooo567
Copy link
Contributor

Actually, it might be better to add this as well #12565 (comment) to avoid the future regression. Can we add some try except that checks if the pid is a string and add some logger.error if that's the case?

@wuisawesome
Copy link
Contributor

Wait I think this is fine. I retract that comment lol. It was uninformed (I didn't knows the context that the original issue was fixed and then resurfaced). I was scared of hunting down deserialization bugs, but this way is more precise.

(@rkooo567 I'm assuming you're ok with this since you approved it, and we can make it more defensive later if we need to).

@wuisawesome wuisawesome merged commit cf1bc66 into ray-project:master Feb 23, 2021
@rkooo567
Copy link
Contributor

I think we need at least a better assertion here. Otherwise we will have regression every time we add a new string pid. I can make a PR real quick for that.

@FarzanT FarzanT deleted the LoggerFix branch February 23, 2021 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log Monitor TypeError
3 participants