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

Option to run process with a nice level (Linux), or lower priority (Windows) #2304

Open
wants to merge 7 commits into
base: nightly
Choose a base branch
from

Conversation

planetrocky
Copy link

@planetrocky planetrocky commented Oct 31, 2024

Description

I've been running this locally since June, and find it useful as it takes hours to full-scan TV Shows.

Add process priority run command --nice/-ni.

This sets process priority to nice(10) on Linux, and BELOW_NORMAL on Windows

Type of Change

  • New feature (non-breaking change which adds functionality)

Checklist

Kometa Wiki would need updating, if this PR is accepted.

  • Updated Documentation to reflect changes
  • Updated the CHANGELOG with the changes

…ority to nice(10) on Linux, and BELOW_NORMAL in Windows
@YozoraXCII YozoraXCII requested a review from chazlarson October 31, 2024 12:50
@YozoraXCII
Copy link
Contributor

Requesting a review from @chazlarson as he will maybe know more about this than I do.

@YozoraXCII YozoraXCII added the status:review-requested An additional review has been requested label Oct 31, 2024
Copy link
Contributor

@chazlarson chazlarson left a comment

Choose a reason for hiding this comment

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

Seems harmless, but needs docs.

@YozoraXCII
Copy link
Contributor

@planetrocky can you add an entry to the Run Commands & Env Variables page? Probably underneath "Screen Width" would be the best place for it:

https://github.com/Kometa-Team/Kometa/blob/nightly/docs/kometa/environmental.md#width

@YozoraXCII YozoraXCII added status:more-info-needed More information is needed and removed status:review-requested An additional review has been requested labels Oct 31, 2024
@chazlarson
Copy link
Contributor

Also, is there an env var version? Are there any complications when running in docker?

@YozoraXCII
Copy link
Contributor

YozoraXCII commented Oct 31, 2024

Also is there a "high priority" method or a way to set a percentage of priority?

@meisnate12
Copy link
Member

pywin32==308 also needs to be added to the requirements.txt

@planetrocky
Copy link
Author

planetrocky commented Nov 1, 2024

@planetrocky can you add an entry to the Run Commands & Env Variables page? Probably underneath "Screen Width" would be the best place for it:

https://github.com/Kometa-Team/Kometa/blob/nightly/docs/kometa/environmental.md#width

I held-off editing that, wasn’t sure if I should or not.

Yes I was going to add WiKi, waiting to see if this PR might go forwards :)

@planetrocky
Copy link
Author

Also, is there an env var version? Are there any complications when running in docker?

KOMETA_ environment variables are automatically created in Kometa.py. I will document :)

@planetrocky
Copy link
Author

Also, is there an env var version? Are there any complications when running in docker?

Docker processes can lower their priority - there’s nothing special required.

CAP_NICE feature is required for a Docker container to set a negative NICE value (high priority above normal) a as that’s normally a root privilege.

Also is there a "high priority" method or a way to set a percentage of priority?

It would complicate user’s Docker setup, in that the cap_add would need SYS_NICE adding. It might also encourage users to run the Docker with raised global privilege (root) if they’re not familiar with the more fine-grained capabilities.

I did consider allowing a value to be specified. My thoughts:

  1. Plex has a setting “run scanner tasks at a lower priority”. This is fixed.
  2. My testing of below normal, normal, above normal, realtime; didn’t show much variation on my system - with the obvious exception that below normal does impact the interactive operation less. A full Kometa run of my Libraries takes maybe 4-5 hours.

Most of my delay is SQLite inside Plex. I’d love to move Plex to Postgres.

@planetrocky
Copy link
Author

pywin32==308 also needs to be added to the requirements.txt

Thanks. I missed that in my checking. I setup a clean Python virtual environment to test. I’ll check it!

@chazlarson
Copy link
Contributor

I bring up the docker situation because people will enable it and then ask about it if it doesn't behave as expected. If there are things that need to be in place for this to make sense in Docker then that needs to be documented either along with the flag and/or in the docker install notes.

@chazlarson
Copy link
Contributor

chazlarson commented Nov 1, 2024

pywin32==308 also needs to be added to the requirements.txt

Seems like that will drive different requirements for linux/osx/docker vs win, which will be another documentation point and will touch all the install docs.

@planetrocky
Copy link
Author

planetrocky commented Nov 1, 2024

I bring up the docker situation because people will enable it and then ask about it if it doesn't behave as expected. If there are things that need to be in place for this to make sense in Docker then that needs to be documented either along with the flag and/or in the docker install notes.

As is; nothing changes, lowering process priority on Docker isn’t privileged - same as a normal user can “nice” their process.

However adding an option for high priority would require changes - and I’d recommend not providing that! :)

@planetrocky
Copy link
Author

pywin32==308 also needs to be added to the requirements.txt

Seems like that will drive different requirements for linux/osx/docker vs win, which will be another documentation point and will touch all the install docs.

I’ll have a think about it.

I find the ability to lower the priority (nice) very useful.

My Kometa has had this change locally for a long time now.

I could just keep as a local patch; but I thought others might find this useful. Especially if running on an under powered NAS.

@chazlarson
Copy link
Contributor

chazlarson commented Nov 1, 2024

pywin32==308 also needs to be added to the requirements.txt

Seems like that will drive different requirements for linux/osx/docker vs win, which will be another documentation point and will touch all the install docs.

I’ll have a think about it.

I find the ability to lower the priority (nice) very useful.

My Kometa has had this change locally for a long time now.

I could just keep as a local patch; but I thought others might find this useful. Especially if running on an under powered NAS.

I am not arguing against it or commenting on the utility of the change at all, just pointing out things that will come up.

If pywin32 is in the requirements.txt, then installing requirements will fail on any non-Windows platform. So that's a new thing that will have to be addressed. Windows [or not-windows] will need a custom requirements.txt, and all the documentation that tells the user to run python -m pip install -r requirements.txt will then need to be made platform-dependent.

Apparently this may not be an issue:

pywin32==308; sys_platform == 'win32'

https://stackoverflow.com/questions/29222269/is-there-a-way-to-have-a-conditional-requirements-txt-file-for-my-python-applica

@planetrocky
Copy link
Author

planetrocky commented Nov 1, 2024

pywin32==308 also needs to be added to the requirements.txt

Seems like that will drive different requirements for linux/osx/docker vs win, which will be another documentation point and will touch all the install docs.

I’ll have a think about it.
I find the ability to lower the priority (nice) very useful.
My Kometa has had this change locally for a long time now.
I could just keep as a local patch; but I thought others might find this useful. Especially if running on an under powered NAS.

I am not arguing against it or commenting on the utility of the change at all, just pointing out things that will come up.

If pywin32 is in the requirements.txt, then installing requirements will fail on any non-Windows platform. So that's a new thing that will have to be addressed. Windows [or not-windows] will need a custom requirements.txt, and all the documentation that tells the user to run python -m pip install -r requirements.txt will then need to be made platform-dependent.

Apparently this may not be an issue:

pywin32==308; sys_platform == 'win32'

https://stackoverflow.com/questions/29222269/is-there-a-way-to-have-a-conditional-requirements-txt-file-for-my-python-applica

I'd already tested the code on Linux and Windows; the conditional import is ignored on Linux. The conditional import is flagged by pylint import-outside-toplevel / C0415

Yes! I was just about to post the same. :)

@planetrocky
Copy link
Author

pywin32==308 also needs to be added to the requirements.txt

Seems like that will drive different requirements for linux/osx/docker vs win, which will be another documentation point and will touch all the install docs.

I’ll have a think about it.
I find the ability to lower the priority (nice) very useful.
My Kometa has had this change locally for a long time now.
I could just keep as a local patch; but I thought others might find this useful. Especially if running on an under powered NAS.

I am not arguing against it or commenting on the utility of the change at all, just pointing out things that will come up.

No absolutely! Totally understand. BTW I've been coding since computer programming was punching holes in cardboard! :)

@planetrocky
Copy link
Author

@planetrocky can you add an entry to the Run Commands & Env Variables page? Probably underneath "Screen Width" would be the best place for it:

https://github.com/Kometa-Team/Kometa/blob/nightly/docs/kometa/environmental.md#width

Screenshot 2024-11-02 033953

@YozoraXCII
Copy link
Contributor

YozoraXCII commented Nov 4, 2024

@planetrocky can you add an entry to the Run Commands & Env Variables page? Probably underneath "Screen Width" would be the best place for it:
https://github.com/Kometa-Team/Kometa/blob/nightly/docs/kometa/environmental.md#width

Screenshot 2024-11-02 033953

Just for clarification:

  • Does --nice work on Windows only? If so the docs should state that
  • Will this work on Windows if Docker is used? If no I would state that and remove the "Docker Environment" tab from the example

And I know it's obvious, but I would highlight that runtimes may be impacted if the user chooses to run it at a lower priority.

@YozoraXCII YozoraXCII removed the request for review from chazlarson November 7, 2024 14:26
@YozoraXCII YozoraXCII dismissed chazlarson’s stale review November 7, 2024 14:27

doc adjustments WIP - awaiting further updates from OP

@YozoraXCII
Copy link
Contributor

@planetrocky can you add an entry to the Run Commands & Env Variables page? Probably underneath "Screen Width" would be the best place for it:
https://github.com/Kometa-Team/Kometa/blob/nightly/docs/kometa/environmental.md#width

Screenshot 2024-11-02 033953

Just for clarification:

  • Does --nice work on Windows only? If so the docs should state that
  • Will this work on Windows if Docker is used? If no I would state that and remove the "Docker Environment" tab from the example

And I know it's obvious, but I would highlight that runtimes may be impacted if the user chooses to run it at a lower priority.

@planetrocky any updates on this please?

@YozoraXCII
Copy link
Contributor

@planetrocky last call to make relevant updates or this PR will be closed.

@YozoraXCII YozoraXCII closed this Dec 10, 2024
@YozoraXCII
Copy link
Contributor

Reopened as closed by accident

@YozoraXCII YozoraXCII reopened this Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:more-info-needed More information is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants