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

feat(controller): 8 - add command line interface for running on OSCAR #347

Closed
wants to merge 39 commits into from

Conversation

hollandjg
Copy link
Member

@hollandjg hollandjg commented Apr 14, 2023

Description

Add a command line interface for running AutoRA jobs.

Add examples showing how they can run asynchronously on Oscar using cylc.

If the issue is on Github, simply link to it using the '#' symbol; otherwise, provide a notion page link):

TODO:

Type of change

Delete as appropriate:

  • feat: A new feature

Features (Optional)

@hollandjg hollandjg requested a review from gtdang April 14, 2023 21:07
@hollandjg hollandjg self-assigned this Apr 14, 2023
Copy link
Collaborator

@gtdang gtdang left a comment

Choose a reason for hiding this comment

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

Oh just realized that this is still in draft. I took a quick look at the typer and logging implementation and added some comments/suggestions.

_configure_logger(debug, verbose)
controller_ = _load_manager(manager)

_logger.debug(f"loading manager state from {directory=}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does the the = after the directory do?

Copy link
Member Author

Choose a reason for hiding this comment

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

that prints "directory=<directory name>" which otherwise you'd have to print like: f"directory={directory}"

Comment on lines +46 to +52
def _configure_logger(debug, verbose):
if debug:
logging.basicConfig(level=logging.DEBUG)
_logger.debug("using DEBUG logging level")
if verbose:
logging.basicConfig(level=logging.INFO)
_logger.info("using INFO logging level")
Copy link
Collaborator

@gtdang gtdang Apr 17, 2023

Choose a reason for hiding this comment

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

Debug level would be deactivated by info level if both debug and verbose are True. Doing if verbose before if debug would make more sense. Debug is actually more verbose than info because it is a lower level than info. Therefore both info and debug statements would be logged at debug level. Because of this I find the argument names are a bit confusing for this function. A single argument for the logging level would be more simple. I've done this with enum as log levels are just encoded integers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you're right that a single argument would be better, and a single logging.basicConfig call. I'll include that in the next version

autora/controller/__main__.py Outdated Show resolved Hide resolved
@hollandjg
Copy link
Member Author

Too big, moving to new repo.

@hollandjg hollandjg closed this Apr 20, 2023
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.

feature: add asynchronous cycle which can deal with long-running evaluations
2 participants