Skip to content

Add dotenv environment variable defaults #2422

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

8W9aG
Copy link
Contributor

@8W9aG 8W9aG commented Jun 20, 2025

  • Allow a .env file to specify the default environment variables to be passed to predict

* Allow a .env file to specify the default
environment variables to be passed to predict
@8W9aG 8W9aG requested review from michaeldwan and aron June 20, 2025 17:37
@michaeldwan
Copy link
Member

michaeldwan commented Jun 20, 2025

What's the use case for this in predict only? Tbh I'd like better support for .env files across the entire cli, but we need to think about the various ways we use env vars, like adding variables (and secrets) to models at build time, or build args, during predict, etc. We just need predictable behavior, specifically, vars are eagerly loaded from a predictable place but explicitly requested before being used.

Just thinking this through, but maybe something like this... first, we add an env-file flag to the root command that defaults to .env. if there's a file present at that path we automatically load it into the env for the command

Later on when passing variables to something, we follow conventions for the K=V string style from other tools (eg docker) to determine if a variable is read from env or not.

  • FOO=bar would set FOO to bar
  • FOO=$FOO or FOO=${FOO} would read FOO from env, and warn or fail if it's missing. This can be offloaded to os.Expand or os.ExpandEnv
  • FOO without =<VAL> would read FOO from the env, and warn/fail if missing.

I think this behavior would be consistent across all the use cases we have (or will be adding) across the CLI without surprises. If this lands in an env package or something we can reuse the logic everywhere.

Thoughts?

@8W9aG
Copy link
Contributor Author

8W9aG commented Jun 20, 2025

What's the use case for this in predict only? Tbh I'd like better support for .env files across the entire cli, but we need to think about the various ways we use env vars, like adding variables (and secrets) to models at build time, or build args, during predict, etc. We just need predictable behavior, specifically, vars are eagerly loaded from a predictable place but explicitly requested before being used.

Just thinking this through, but maybe something like this... first, we add an env-file flag to the root command that defaults to .env. if there's a file present at that path we automatically load it into the env for the command

Later on when passing variables to something, we follow conventions for the K=V string style from other tools (eg docker) to determine if a variable is read from env or not.

  • FOO=bar would set FOO to bar
  • FOO=$FOO or FOO=${FOO} would read FOO from env, and warn or fail if it's missing. This can be offloaded to os.Expand or os.ExpandEnv
  • FOO without =<VAL> would read FOO from the env, and warn/fail if missing.

I think this behavior would be consistent across all the use cases we have (or will be adding) across the CLI without surprises. If this lands in an env package or something we can reuse the logic everywhere.

Thoughts?

Use case is supplying a replicate api token which is a runtime problem rather than a build problem, see DP-280 for details. This would be different to how cog.yaml defines them due to that being a build problem. Having said that I'm open to making this a CLI option where people can specify the use of this file even for build time concerns.

@michaeldwan
Copy link
Member

Ah I understand, that makes sense! So yeah, standardizing cog env var loading is out of scope, and I'd prefer we do that properly in isolation.

Would this work for now: automatically read the .env file, but instead of loading it into the process env, load it as a map and automatically pluck out just the values you need. If REPLICATE_API_TOKEN/REPLICATE_USERNAME/etc are present, pass them to the model for predict, serve, and whatever else needs it.

Also, this is like the 3rd time this week where I've found a good reason for cog to maintain a state file for config. Only using the docker auth for registry token is making things needlessly difficult.

@aron
Copy link
Contributor

aron commented Jun 24, 2025

Would this work for now: automatically read the .env file, but instead of loading it into the process env, load it as a map and automatically pluck out just the values you need. If REPLICATE_API_TOKEN/REPLICATE_USERNAME/etc are present, pass them to the model for predict, serve, and whatever else needs it.

As a user I think this would be unexpected. I'd expect the .env to make those variables available to my running app e.g. cog serve and cog predict. Though I get that the use case is different for cog build and that we wouldn't want to accidentally slurp up local env variables accidentally. If we want to limit support to only known variables a config file would be more appropriate.

The primary goal here is to allow a user to run cog predict many times without having to specify command line flags each time. Environment variables felt like a well understood approach.

@8W9aG
Copy link
Contributor Author

8W9aG commented Jun 25, 2025

I agree I'm not sure it makes sense to scope the .env file to just cherrypick environment variables, this will be surprising to users. @michaeldwan is there a precedent for .env files being used in build rather than run workflows?

@michaeldwan
Copy link
Member

Sorry I missed your reply @aron! You're both right, env variables are the right thing to use. I'm only (gently) pushing back on scope and timing. Supporting .env files, and host-to-container env in general, comes with ux footguns we need to think through. I haven't thought through it more than the discussion above, so I'll think it through in words here and we can figure it out together 🙃

The main question is if .env is to configure cog, or to configure the container. Loading .env to the cog process is fine, and loading it to the container is also fine. But using a single file to configure both is the footgun I'm stuck on.


The default/expected behavior of an .env file is to get loaded into the process environment, which makes sense. We want the token in os.Environ() so cog can access it, and we probably want it in the container as well. NBD. We're probably fine to pass all the other COG_ or R8_* variables to the container as well.

What we don't want is to load .env into the current process environment and then pass the current env to the container. Most of the variables would be fine in the container, but obviously we don't want to overwrite things like PATH/USER/SHELL/HOME which would break the container, or config like HTTP_PROXY or SSH_* which would probably not break it but behavior would be unexpected. The way docker-like tooling contains the blast radius is requiring explicit ENV mapping (-e KEY, -e KEY=$HOSTKEY, or --env-file .env) when containers run. And this is a build-time concern as well since build is just a series of running containers.

There's also a number of collisions I'd expect between cog and the container, like LOG_LEVEL. Or configuring python with UV_* that would impact the host, the build env, and the runtime env.

We can go into more detail with all that, but that's the gist.

So what options do we have for a path forward? The quickest is to not address this yet and use .env as a convenience for the token until we can properly come back and think this through. But if we do want to move forward with making env variables available in the container, perhaps we should consider multiple files. For example, .env autoloads on the host (expected behavior) and .env.cog autoloads in the container, and we can even support .env.cog.build or similar to target variables for a specific specific stage.

Dunno... just wanted to dump some thoughts for discussion. What do y'all think?

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.

3 participants