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

Fallback to default profile when profiles file is empty #16119

Merged
merged 2 commits into from
Nov 26, 2024
Merged

Conversation

desertaxle
Copy link
Member

@desertaxle desertaxle commented Nov 26, 2024

This PR adds handling to the profile settings source to load the default ephemeral profile if another profile can't be loaded.

Closes #16101
Closes #16118
supersedes #16112

@github-actions github-actions bot added the bug Something isn't working label Nov 26, 2024
Copy link

codspeed-hq bot commented Nov 26, 2024

CodSpeed Performance Report

Merging #16119 will not alter performance

Comparing fix-16101 (bc66125) with main (62b1001)

Summary

✅ 3 untouched benchmarks

Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

lgtm!

@dsmfreire
Copy link

This doesn't work if, for example, the ephemeral default profile has multiple variables and the user changes a subset of them.

E.g.
default profiles.toml

[profiles.ephemeral]
PREFECT_VAR_1 = "true"
PREFECT_VAR_2 = "true"

~/.prefect/profiles.toml

[profiles.ephemeral]
PREFECT_VAR_1 = "false"

In this case, PREFECT_VAR_2 would evaluate to the settings' class default, not the value from the default profiles.toml file.

Wouldn't it be a better solution to merge the two, overriding defaults with user settings but maintaining the default loaded?

@desertaxle
Copy link
Member Author

Hey @dsmfreire! We want user-defined profiles to take precedence over default profiles and I think that merging user-defined profiles with default profiles has the potential to be more confusing.

We handle users making changes to the default profiles by copying the values for that profile into the user's profiles.toml file when they make their first change via prefect config set.

For example, if I run prefect config set PREFECT_LOGGING_LEVEL="DEBUG" in a brand new install, I end up with a profiles.toml that looks like this:

active = "ephemeral"

[profiles.ephemeral]
PREFECT_SERVER_ALLOW_EPHEMERAL_MODE = "true"
PREFECT_LOGGING_LEVEL = "DEBUG"

where I have ephemeral mode enabled even though I haven't explicitly enabled it via the CLI because that came with the default profile.

This way, the config is visible to the user, which I think is less confusing overall.

I'm happy to discuss this more or other scenarios! I'm going to merge this PR now, so either a new issue or GitHub discussion would be a great place to chat!

@desertaxle desertaxle merged commit 11eef8a into main Nov 26, 2024
41 checks passed
@desertaxle desertaxle deleted the fix-16101 branch November 26, 2024 19:23
@dsmfreire
Copy link

Hey @desertaxle! Thank you for the response. I was unaware of the "config set" behavior of copying the defaults to the user's profiles.toml. That's a great solution and way less confusing than what I suggested, so I retract my previous comment. Sorry for the ignorance, and thanks!

@desertaxle
Copy link
Member Author

No worries, thanks for bringing it up! I'm always happy to discuss DX improvements!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants