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

Respect convert's threads configuration #71

Merged
merged 5 commits into from Jun 5, 2023

Conversation

johnyerhot
Copy link
Contributor

Small PR that adds support for respecting convert's threads config. #41.

I didn't write any tests so that's now awesome but Works on My Machine™.

The code located here is probably also not needed or could be refactored to remove setting threads to CPU count since that should already be done via the convert plugin.

@geigerzaehler
Copy link
Owner

Thanks for the contribution @johnyerhot! Before this gets merged could you also construct the Worker with the right number of threads in the this location? Could you also make sure your code is properly formatted by running poetry run black .?

The code located here is probably also not needed or could be refactored to remove setting threads to CPU count since that should already be done via the convert plugin.

You’re right. Would you care to fix this, too?

@johnyerhot
Copy link
Contributor Author

johnyerhot commented Jun 1, 2023

Before this gets merged could you also construct the Worker with the right number of threads in the this location?

Doh! Yep, did it!

Could you also make sure your code is properly formatted by running poetry run black .?

Done and done.

You’re right. Would you care to fix this, too?

Done and done. I believe we can completely remove all references to it and assume there will be a value in max_workers since we're directly sucking in the value from its threads config. The will always be a value or cpu threads.

All behaved as expected in my testing. Thanks for the help @geigerzaehler ❤️

@geigerzaehler
Copy link
Owner

Thanks for the follow-up! The PR is missing one final formatting touch: poetry run isort .. Once you’ve fixed this I’ll merge it and release a new version.

@johnyerhot
Copy link
Contributor Author

Done and done @geigerzaehler!

@geigerzaehler geigerzaehler merged commit ba69bfd into geigerzaehler:master Jun 5, 2023
4 of 5 checks passed
@geigerzaehler
Copy link
Owner

Released in v0.11.0

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.

None yet

2 participants