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

add high-assurance placeholder option to endpoint configuration #1720

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LeiGlobus
Copy link
Contributor

@LeiGlobus LeiGlobus commented Nov 13, 2024

This is the skeleton of adding high-assurance to endpoint configuration.

The option in globus-compute-endpoint configure --high-assurance is marked as hidden for now.

Like multi-user, the config is only present in the config file if it is True. The web service treats missing values as False as well.

Obviously no changelog fragment for now.

For a bit more context see SC https://github.com/globusonline/funcx-services/pull/726

V3 register adds the high_assurance option if provided, V2 does not support registration with the value set to True. However GET /v2/endpoint does return the value.

@LeiGlobus LeiGlobus added the no-news-is-good-news This change does not require a news file label Nov 13, 2024
@LeiGlobus LeiGlobus marked this pull request as draft November 13, 2024 20:42
@LeiGlobus LeiGlobus marked this pull request as ready for review November 13, 2024 21:21
@LeiGlobus LeiGlobus force-pushed the ha-endpoints-sc-35308 branch from 9b54d36 to abb4a3a Compare November 18, 2024 18:54
@LeiGlobus LeiGlobus force-pushed the ha-endpoints-sc-35308 branch from abb4a3a to e4fd088 Compare November 18, 2024 19:55
Comment on lines 6 to 8
display_name=None, # If None, defaults to the endpoint name
high_assurance=False,
executors=[
Copy link
Contributor

Choose a reason for hiding this comment

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

If it defaults to False, per config.py, why include this at all in the default_config.py? Especially since this is yet a hidden feature (per "in-development!") given cli.py?

Comment on lines +86 to +89
if ha is True:
assert "high_assurance" in config_dict
else:
assert "high_assurance" not in config_dict
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be reduced to:

assert ("high_assurance" in config_dict) is (ha is True)

Comment on lines 45 to +47
class BaseConfigModel(BaseModel):
multi_user: t.Optional[bool]
high_assurance: t.Optional[bool]
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, need to rebase on main, we just updated this per the 2.32.1 bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-news-is-good-news This change does not require a news file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants