uv
Add support for `.env` and custom env files in `uv run`
#8263
Merged

Add support for `.env` and custom env files in `uv run` #8263

edugzlez
edugzlez221 days ago👍 4

Summary

I have been reading discussion #1384 about .env and how to include it in the uv run command.

I have always wanted to include this possibility in uv, so I was interested in the latest changes. Following @charliermarsh's advice I have tried to respect the philosophy that uv run uses the default .env and this can be discarded or changed via terminal or environment variables.

The behaviour is as follows:

  • uv run file.py executes file.py using the .env (if it exists).
  • uv run --env-file .env.development file.py uses the .env.development file to load the variables and then runs file.py. In this case the program fails if the file does not exist.
  • uv run --no-env-file file.py skips reading the .env file.

Equivalently, I have included the UV_ENV_FILE and UV_NO_ENV_FILE environment variables.

Test Plan

I haven't got into including automated tests, I would need help with this.

I have tried the above commands, with a python script that prints environment variables.

zanieb zanieb requested a review from charliermarsh charliermarsh 221 days ago
charliermarsh charliermarsh assigned charliermarsh charliermarsh 221 days ago
charliermarsh charliermarsh added this to the v0.5.0 milestone 221 days ago
charliermarsh
charliermarsh221 days ago🚀 1

This generally looks good, but I think we should bundle it in v0.5.0, since it is a change in behavior.

charliermarsh
charliermarsh221 days ago👍 1

Do you mind adding a test to run.rs too, to validate that the files are being loaded and passed to the runtime?

charliermarsh
charliermarsh commented on 2024-10-16
Conversation is marked as resolved
Show resolved
crates/uv/src/commands/project/run.rs
106108
109 // Initialize env file, if necessary.
110 if !no_env_file {
111
let env_file_path = env_file.clone().unwrap_or(Path::new(".env").to_path_buf());
charliermarsh221 days ago👍 1

You can use unwrap_or_else to avoid allocating (.to_path_buf()) in the event that env_file is provided.

edugzlez221 days ago

Good point! Thanks!

edugzlez
edugzlez221 days ago (edited 221 days ago)

Thank you very much for your advice and input.

The coding experience when writing tests is fantastic, I didn't expect it to be so good.

There seems to be a problem running the tests, but it escapes my understanding. I've seen that other PRs are having the same error.

j178
j178 commented on 2024-10-17
crates/uv/src/commands/project/run.rs
j178220 days ago

I think this clone is not necessary?

edugzlez220 days ago

This is necessary because you then need to use env_file to check whether it is passed as an argument or the default argument is taken.

if env_file.is_some() {

Maybe I could change it to this:

let is_env_file_present = env_file.is_some();
let env_file_path = env_file.unwrap_or_else(|| Path::new(".env").to_path_buf());

let loaded = dotenvy::from_path_override(&env_file_path);

match loaded {
    Err(dotenvy::Error::Io(err)) => {
        if is_env_file_present {
            bail!(
                "Failed to read environment file `{}`: {}",
                env_file_path.display(),
                err
            );
        }
    }

to avoid the clone (but at the cost of creating a variable first).

I'm not sure.

charliermarsh215 days ago

I was able to remove the clone, if you're interested in seeing how.

edugzlez edugzlez requested a review from charliermarsh charliermarsh 219 days ago
charliermarsh
charliermarsh219 days ago🎉 4

We're planning to do v0.5 next week (hopefully) which would include this.

edugzlez add --env-file and --no-env-file to uv run
4edab09b
edugzlez docs for --env-file and --no-env-file
7084d68f
edugzlez prettier
2868b499
edugzlez use unwrap_or_else to avoid allocating
e793070e
edugzlez fix warning message when environment file parsing fails
fb86182d
edugzlez add tests for --env-file and --no-env-file
d6c061e5
edugzlez fix clippy errors
d3176f52
charliermarsh charliermarsh changed the base branch from main to tracking/050 215 days ago
charliermarsh charliermarsh force pushed from bd4dcd4d to d3176f52 215 days ago
charliermarsh
charliermarsh215 days ago🚀 4

Will get this merged into the v0.5 branch tomorrow.

charliermarsh Minor tweaks
37df38d4
charliermarsh charliermarsh force pushed from 19e1051f to ac25501b 215 days ago
charliermarsh
charliermarsh approved these changes on 2024-10-22
charliermarsh charliermarsh force pushed from ac25501b to c0fb81f6 215 days ago
charliermarsh charliermarsh force pushed from c0fb81f6 to 91cc54fc 215 days ago
charliermarsh charliermarsh force pushed from 91cc54fc to b02fbdff 215 days ago
charliermarsh charliermarsh force pushed from b02fbdff to 8b6bbb0b 215 days ago
charliermarsh Add docs
e5fee968
charliermarsh charliermarsh force pushed from 8b6bbb0b to e5fee968 215 days ago
charliermarsh charliermarsh merged f6d9804f into tracking/050 215 days ago
AndreuCodina
AndreuCodina215 days ago (edited 215 days ago)

@charliermarsh

Sorry, but I have to comment before this is released.

Backends in serious projects have several sources: you use the pydantic-settings package or the equivalent in another language, and there you define the sources and their priority.

What do I mean by "sources"?
For example, when the backend starts, the backend itself configures the sources. In the code below, the Pydantic model must be filled with values of several sources. First, it reads from Azure Key Vault if you aren't in localhost, then it reads .env.MyEnvironment files (overriding the value if the same variable is found), and then it reads from environment variables (overriding the value if the same variable is found). You can add more sources, change the order and change their priority (https://docs.pydantic.dev/latest/concepts/pydantic_settings/#other-settings-source).

from azure.identity import DefaultAzureCredential
from pydantic import Field
from pydantic_settings import (
    AzureKeyVaultSettingsSource,
    BaseSettings,
    DotEnvSettingsSource,
    PydanticBaseSettingsSource,
    SettingsConfigDict,
)

from common.application_environment import ApplicationEnvironment


class ApplicationSettings(BaseSettings):
    webhook_url: str
    cosmosdb_connection_string: str

    model_config = SettingsConfigDict(extra="ignore")

    @classmethod
    def settings_customise_sources(
        cls,
        settings_cls: type[BaseSettings],
        init_settings: PydanticBaseSettingsSource,
        env_settings: PydanticBaseSettingsSource,
        dotenv_settings: PydanticBaseSettingsSource,
        file_secret_settings: PydanticBaseSettingsSource,
    ) -> tuple[PydanticBaseSettingsSource, ...]:
        env = EnvSettingsSource(
            settings_cls, env_nested_delimiter="__", case_sensitive=True
        )
        dotenv = DotEnvSettingsSource(
            settings_cls,
            env_file=[".env", f".env.{ApplicationEnvironment.get_current()}"],
            env_nested_delimiter="__",
            case_sensitive=True,
        )
        azure_key_vault = AzureKeyVaultSettingsSource(
            settings_cls,
            dotenv()["AzureKeyVaultUrl"],
            DefaultAzureCredential(),
        )
        settings = (env, dotenv)

        if ApplicationEnvironment.get_current() != ApplicationEnvironment.LOCAL:
            settings += (azure_key_vault,)

        return settings

Furthermore, .env.Production is read from the production environment, but also .env because the variables in .env and shared through all .env.* files.

Plus, I don't want to run uv run --env-file .env.Development, just uv run.

I'm not a big fan of how pydantic-settings is done: it should provide good defaults and an interface with less boilerplate, but it must be done with code.

The code should declare the sources, not a runner tool such as uv, coupling to a single source (uv only reads .env files, and specific .env files) and affecting the code. This PR shouldn't be released.

charliermarsh
charliermarsh215 days ago👍 2

It's really common for runners to support .env files -- Node does it, Bun does it, Deno does it. If you don't want the feature to be enabled, you can just opt out of it!

AndreuCodina
AndreuCodina215 days ago (edited 215 days ago)

I'm not a NodeJS developer, but this PR is breaking conventions and good practices.

Developers always need to load the .env.Local values when they execute the code in localhost. Why do they have to use uv run --env-file .env.Local instead of just uv run? It doesn't make sense to me, even if it's optional.

But also, the code loses the control to manage the sources, their priority and their order because the runner tool decided to load values from a configuration file by default.

charliermarsh
charliermarsh215 days ago

Developers always need to load the .env.Local values when they execute the code in localhost. Why do they have to use uv run --env-file .env.Local instead of just uv run? It doesn't make any sense for me.

I don't understand this critique. Are you asking for uv run to automatically load anything that's prefixed with .env? Or are you asking that uv run never detect these?

But also, the code loses the control to manage the sources, their priority and their order because the runner tool decided to load values from a configuration file by default.

Again: I'm sorry you feel that way but you can just disable this feature if you don't want it. A more reasonable critique would be to say that this should be opt-in rather than opt-out.

AndreuCodina
AndreuCodina215 days ago (edited 215 days ago)

I don't understand this critique. Are you asking for uv run to automatically load anything that's prefixed with .env? Or are you asking that uv run never detect these?

Never. Imagine this case:

You have three environments (.env.Development, .env.Staging, .env.Production) and optionally localhost (.env.Local). pydantic-settings reads the .env file to get shared settings, and then read an environment variable (to detect the environment name) to load the proper .env file (.env.Local for example).

Another example: in .NET, by default, the environment variables have priority over configuration files (https://learn.microsoft.com/en-us/aspnet/core/fundamentals/configuration/?view=aspnetcore-8.0#default-application-configuration-sources). At this point, this PR is mixing 2 sources into one, and they are different.

When you develop a backend, you must read from several sources, and therefore you need pydantic-settings or another library. You have to load secrets from Azure Key Vault (they can't be in configuration files), auto-reloadable configurations from Azure App Configuration, configuration files, environment variables, etc.

So, you have to load from several sources.

What's an example of a shared configuration? The number of messages to read from a queue, the server to log in against, the timeout of a webhook... And sometimes you need to override it, for example, modifying the App Service environment variable, Dockerfile, App Configuration...

This PR adds the capability of, by default, reading from a source (a configuration file) and loading its values into another source (the environment variables).

Again: I'm sorry you feel that way but you can just disable this feature if you don't want it. A more reasonable critique would be to say that this should be opt-in rather than opt-out.

I'm against adding this because instead of advantages or options, it adds a feature that I'm pretty sure how it'll be used: instead of adding a library such as pydantic-settings, developers will add parameters to uv to read from configuration files, and they'll load the secrets modifying the cloud, adding environment variables by hand.

But yes, my issue is uv is reading from the .env file by default when I think it shouldn't.

pantheraleo-7
pantheraleo-7215 days ago👍 3

A more reasonable critique would be to say that this should be opt-in rather than opt-out.

This (reading .env file) is a great feature, but not a sensible default. It should be opt-in.

If the environment is not loaded programmatically, uv run doing it (by default, without any flags) may cause different behaviour when running it without uv and also it will be too much "magic BTS".

Explicit is better than implicit.
~ Zen of Python, PEP20

seapagan
seapagan215 days ago👍 1😕 1

A more reasonable critique would be to say that this should be opt-in rather than opt-out.

This (reading .env file) is a great feature, but not a sensible default. It should be opt-in.

If the environment is not loaded programmatically, uv run doing it (by default, without any flags) may cause different behaviour when running it without uv and also it will be too much "magic BTS".

Explicit is better than implicit.

~ Zen of Python, PEP20

Opt-in as it's technically not backwards compatible and may be unexpected - and I'd argue the majority of users won't use this. In production my apps may need to read .env vars (though should be done by actual vars at that stage) but they are never RUN through 'uv'. So they need to include dotenv or whatever anyway.

My big issue is transparency- some env vars can change the actual paths and options of that process - I don't want to run 'uv' in a repo I've just cloned and have it QUIETLY set env vars I don't know about.

Opt in please. Make it a global setting that is set and forget, but please don't do it behind our backs.

charliermarsh
charliermarsh215 days ago👍 3

(Not disagreeing with your broader point, but I'll just note that this is already marked as a breaking change that would be included in v0.5 per our versioning policy. It's ok for it not to be backwards compatible.)

charliermarsh charliermarsh added breaking
charliermarsh charliermarsh added enhancement
seapagan
seapagan215 days ago

Fair point 😃

My points still stand though. There is a reason that 'direnv' REQUIRES you to physically allow auto-reading an env file the first time - security.

Maybe do this for each new project the first time? At least give the user the choice. Otherwise a global option, but please, Opt in.

zanieb
zanieb215 days ago👍 2

My big issue is transparency- some env vars can change the actual paths and options of that process - I don't want to run 'uv' in a repo I've just cloned and have it QUIETLY set env vars I don't know about.

I don't know if this is a fair concern. If you clone an arbitrary repository and execute uv run in it, you're already trusting the repository. They can execute arbitrary code at that point. direnv is different because it hooks into your shell and affects any command you run in the directory (i.e. the repository can be untrusted and you can still operate on it).

seapagan
seapagan215 days ago

I don't know if this is a fair concern. If you clone an arbitrary repository and execute uv run in it, you're already trusting the repository. They can execute arbitrary code at that point. direnv is different because it hooks into your shell and affects any command you run in the directory (i.e. the repository can be untrusted and you can still operate on it).

This is very true yes. Maybe it's just my prejudice showing, and I really have no use-case for it. In many cases my code is running uvicorn-> guniciorn -> nginx so 'uv' is not in the pipe and any .env would need to be loaded manually. If I'm using an env file, I'd make sure my code explicitly reads it, and that is really more Zen.

I truly believe you may end up with new issues related to unexpected env vars being set - how many people really read the release notes or just go 'yeah yeah, upgrade'😜.

I'll stand by my 'opt-in' recommendation but obv not going to stop using uv if you choose otherwise.

seapagan
seapagan215 days ago

One more thing - anyone who wants and uses this feature should have no problem opting in?🤷‍♂️

TheRealBecks
TheRealBecks214 days ago

After the migration from pipenv to uv our local Ansible environments are broken as uv does not read the .env file by default.

Also Docker and Ansible read .env by default.

In my case (what our company does): The .env files are only needed for local development where users also run uv run ... manually. Opt-in would be no comfort from the users perspective especially when projects will be migrated from tools like pipenv to uv. Also production systems get different environment variables that will be set for the run and will not use any .env file and if I would like to change that behaviour I will use the UV_xx environment variables.

So, maybe this approach will be accepted:

  1. Use opt-out by default, so uv reads the .env file by default
  2. Add UV_xx environment variables for not reading and reading .env files: You already planned this feature
  3. Add another option for the pyproject.toml file so reading an .env file for a project can be set to opt-in instead of the default opt-out

@seapagan @AndreuCodina Would option 3 something you would like to see in uv?

seapagan
seapagan212 days ago👍 1

I'd still ask kindly if you can add a setting to the uv.toml so we can disable this globally please 🙏

artificial-aidan
artificial-aidan210 days ago

Suggestion here to make the Read .env file debug output info or something else visible more easily. It will help with the transition, and prevent confusion of people not realizing the silent behavior.

Login to write a write a comment.

Login via GitHub

Reviewers
Assignees
Labels
Milestone