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.

Sign up for GitHub

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

Jump to bottom

[server] 4/n Update Use .env at the run step #1384

Merged
merged 3 commits into from
Mar 4, 2024
Merged

[server] 4/n Update Use .env at the run step #1384

merged 3 commits into from
Mar 4, 2024

Conversation

Ankush-lastmile
Copy link
Member

@Ankush-lastmile Ankush-lastmile commented Feb 29, 2024 •
edited
Loading

[server] 4/n Update Use .env at the run step

Summary:

Test Plan:

Stack created with Sapling. Best reviewed with ReviewStack.

This was referenced Feb 29, 2024
Merged
Merged
Ankush-lastmile
Ankush-lastmile commented Feb 29, 2024
View reviewed changes
python/src/aiconfig/editor/server/server.py Outdated
@@ -334,7 +334,8 @@ def run() -> FlaskResponse:

# Allow user to modify their environment keys without reloading the server.
# Execution time of `0.001s` is arbitrary, but should be small enough to not be noticeable.
dotenv.load_dotenv()
# Should override be set to True?
dotenv.load_dotenv(state.env_path)
Copy link
Member Author

@Ankush-lastmile Ankush-lastmile Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

RFC: should we override when a new key is set? unsure

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. I say yes, we should override if env_path is set.

If we don't override, then calling set_env_file_path will effectively be a no-op if the file contains variables that already exist (e.g. from a previous call). This would be confusing in a scenario where we call set_env_file_path with one file and then call it again with another. From the server standpoint, I would say:

  • if no state.env_path, then don't override (keep existing behaviour)
  • if state.env_path is set, there has been an explicit request to set env path and would be ideal for the variables in that env file to take precedence

But, from the vscode perspective, is there any scenario where set_env_file_path will be called with different paths for the same server? Is the server persisted when moving a file to a different dir, for example? If there is such a case I think we must override if the path is set, otherwise we would not be using the closest/expected env variables. I guess the fact we have a 'set API keys' command means this is the case.

More generally, the only way I could make an argument to not override is:

  • if it can't be changed for an existing server
  • user is familiar with load_dotenv so they know existing environment (e.g. set in shell) takes precedence over .env file

IMO this argument isn't very strong and as a user if I explicitly do an action to "Set API Keys" I would 100% want the key I just set to be used instead of whatever may already be set for the same variable

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the analysis. I'm in agreement. In the meantime I've updated to set override to true

@Ankush-lastmile Ankush-lastmile changed the title 4/n Use .env at the run step [server] 4/n Update Use .env at the run step Feb 29, 2024
Ankush-lastmile pushed a commit that referenced this pull request Feb 29, 2024
[server] 2/n Add env path to Server State
f3ecbdf 
Summary:


Title. No functional Changes in this diff.

Test Plan:

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/1384).
* __->__ #1384
* #1383
* #1382
@Ankush-lastmile Ankush-lastmile mentioned this pull request Feb 29, 2024
Merged
[server] 2/n Add env path to Server State
da63f58 
Summary:

Stack contains changes that updates overall environment setup flow for vscode extension.

No functional Changes in this diff.

Test Plan:
@Ankush-lastmile Ankush-lastmile marked this pull request as ready for review February 29, 2024 21:49
@Ankush-lastmile
Copy link
Member Author

  • set override to true

This was referenced Feb 29, 2024
Merged
Merged
rholinshead
rholinshead approved these changes Feb 29, 2024
View reviewed changes
Copy link
Contributor

@rholinshead rholinshead left a comment

Choose a reason for hiding this comment

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

Accepting to unblock, but please make suggested changes

python/src/aiconfig/editor/server/server.py Outdated
@@ -335,7 +335,8 @@ def run() -> FlaskResponse:

# Allow user to modify their environment keys without reloading the server.
# Execution time of `0.001s` is arbitrary, but should be small enough to not be noticeable.
dotenv.load_dotenv()
# Should override be set to True?
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe update comment to state something like,
"Override if env_file_path is specified so that provided .env values take precedence over existing values"

python/src/aiconfig/editor/server/server.py Outdated
@@ -335,7 +335,8 @@ def run() -> FlaskResponse:

# Allow user to modify their environment keys without reloading the server.
# Execution time of `0.001s` is arbitrary, but should be small enough to not be noticeable.
dotenv.load_dotenv()
# Should override be set to True?
dotenv.load_dotenv(state.env_file_path, override= True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do override=(state.env_file_path is not None) (or whatever the clean python way is). We should not override if no path is set since that changes behaviour

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, +1. You're pretty good at python 😉

Ankush Pala [email protected] added 2 commits March 4, 2024 10:56
[server] 3/n Endpoint for set_env
dd3c4f3 
Summary:

Stack contains changes that updates overall environment setup flow for vscode extension.

This diff implements the server side endpoint. Will test on top with vscode

Test Plan:
```
payload={'env_file_path': {}}
Status Code: 400
Response JSON: {'message': 'Invalid request, specified .env file path is not a string.'}

payload={'env_file_path': '/hi'}
Status Code: 400
Response JSON: {'message': 'Specified .env file path does not exist.'}

payload={'env_file_path': '.env'}
Status Code: 200
Response JSON: {'message': 'Updated .env file path.'}
```
[server] 4/n Update Use .env at the run step
e858d2b 
Summary:

Test Plan:
@Ankush-lastmile
Copy link
Member Author

  • added a comment
  • override to be set to True only if an env file path is specified, as per reasoning in @rholinshead 's comment

@Ankush-lastmile Ankush-lastmile merged commit 25b6bf5 into main Mar 4, 2024
1 check passed
Ankush-lastmile pushed a commit that referenced this pull request Mar 4, 2024
[server] 5/n Update AIConfig edit|start commands to pass env
37e936d 
Summary:

This diff modifies the AIConfig start and edit commands to take in an dot env path.

Due to core utils logic of handling args, the help messages automatically get updated too.

Also Removes the loadenv() on server init. This is okay because loadenv is invoked at the run step. [see 4/n](#1384)

Test Plan:
Ankush-lastmile pushed a commit that referenced this pull request Mar 4, 2024
[server] 5/n Update AIConfig edit|start commands to pass env
1932b1e 
Summary:

This diff modifies the AIConfig start and edit commands to take in an dot env path.

Due to core utils logic of handling args, the help messages automatically get updated too.

Also Removes the loadenv() on server init. This is okay because loadenv is invoked at the run step. [see 4/n](#1384)

Test Plan:

```
ankush@ap-mbp aiconfig % aiconfig start -h
/opt/homebrew/lib/python3.11/site-packages/pydantic/_internal/_fields.py:151: UserWarning: Field "model_parsers" has conflict with protected namespace "model_".

You may be able to resolve this warning by setting `model_config['protected_namespaces'] = ()`.
  warnings.warn(
usage: aiconfig start [-h] [--server-port SERVER_PORT] [--log-level LOG_LEVEL] [--server-mode {debug_servers,debug_backend,prod}] [--parsers-module-path PARSERS_MODULE_PATH] [--env-file-path ENV_FILE_PATH]

options:
  -h, --help            show this help message and exit
  --server-port SERVER_PORT
  --log-level LOG_LEVEL
  --server-mode {debug_servers,debug_backend,prod}
  --parsers-module-path PARSERS_MODULE_PATH
  **--env-file-path ENV_FILE_PATH**
```
Ankush-lastmile pushed a commit that referenced this pull request Mar 5, 2024
[server] 5/n Update AIConfig edit|start commands to pass env
3e14a8f 
Summary:

This diff modifies the AIConfig start and edit commands to take in an dot env path.

Due to core utils logic of handling args, the help messages automatically get updated too.

Also Removes the loadenv() on server init. This is okay because loadenv is invoked at the run step. [see 4/n](#1384)

Test Plan:

```
ankush@ap-mbp aiconfig % aiconfig start -h
/opt/homebrew/lib/python3.11/site-packages/pydantic/_internal/_fields.py:151: UserWarning: Field "model_parsers" has conflict with protected namespace "model_".

You may be able to resolve this warning by setting `model_config['protected_namespaces'] = ()`.
  warnings.warn(
usage: aiconfig start [-h] [--server-port SERVER_PORT] [--log-level LOG_LEVEL] [--server-mode {debug_servers,debug_backend,prod}] [--parsers-module-path PARSERS_MODULE_PATH] [--env-file-path ENV_FILE_PATH]

options:
  -h, --help            show this help message and exit
  --server-port SERVER_PORT
  --log-level LOG_LEVEL
  --server-mode {debug_servers,debug_backend,prod}
  --parsers-module-path PARSERS_MODULE_PATH
  **--env-file-path ENV_FILE_PATH**
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rholinshead rholinshead rholinshead approved these changes

@saqadri saqadri Awaiting requested review from saqadri saqadri is a code owner

@suyoglastmileai suyoglastmileai Awaiting requested review from suyoglastmileai

@jonathanlastmileai jonathanlastmileai Awaiting requested review from jonathanlastmileai jonathanlastmileai is a code owner

@rossdanlm rossdanlm Awaiting requested review from rossdanlm rossdanlm is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Ankush-lastmile @rholinshead