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

Replace settings variable with function #134

Merged
merged 2 commits into from Mar 1, 2022
Merged

Conversation

SimonBoothroyd
Copy link
Contributor

@SimonBoothroyd SimonBoothroyd commented Mar 1, 2022

Description

This PR replaces the 'dangerous' settings field exposed by the bespoke executor with a more reliable function that returns the most up-to-date values.

The object returned by current_settings() should not be modified as the changes will have no effect. Instead the apply_env context manager should be used e.g.

with Settings(
    BEFLOW_FRAGMENTER_WORKER_N_CORES=...,
    BEFLOW_FRAGMENTER_WORKER_MAX_MEM=...
).apply_env():

    settings = current_settings()

Possibly fixes #130

Status

  • Ready to go

db=settings.BEFLOW_REDIS_DB,
host=__settings.BEFLOW_REDIS_ADDRESS,
port=__settings.BEFLOW_REDIS_PORT,
db=__settings.BEFLOW_REDIS_DB,
)
celery_app = configure_celery_app("qcgenerator", redis_connection)
Copy link
Contributor

Choose a reason for hiding this comment

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

The info in the redis_connection object seems to be the same as that in the settings class and this logic seems to be repeated for each app, would it make sense to have the configure_celery_app just call the current_settings function and get the relavent info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think either having configure celery return a tuple of both the app and redis connection, or adding a new configure_redis method that also checks for the executor key you mentioned in slack / any other validation would be a great shout!

I think I have a slight preference for the latter, but not by much 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably handle this in a new PR though so it doesn't get buried here! 😅

Copy link
Contributor

@jthorton jthorton left a comment

Choose a reason for hiding this comment

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

@SimonBoothroyd thanks for the quick fix, can confirm this now works on linux as expected!

@SimonBoothroyd SimonBoothroyd merged commit 410d766 into main Mar 1, 2022
@SimonBoothroyd SimonBoothroyd deleted the remove-settings-field branch March 1, 2022 19:10
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.

Executor run CLI not respecting --qc-compute-n-cores
2 participants