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 use_env_var flag to client #923

Merged
merged 11 commits into from
Dec 22, 2022
Merged

Conversation

loomlike
Copy link
Collaborator

Signed-off-by: Jun Ki Min 42475935+loomlike@users.noreply.github.com

Description

Introduce an option to select between env vars and config yaml file.
Feathr client to use explicitly configured yaml file over environment variable if use_env_var flag is set to False.

The changes are added as the last argument of the existing functions and set default to True (use env variables) so that
existing codes don't break.

Resolves #922

How was this PR tested?

Unit test

Does this PR introduce any user-facing changes?

  • No. You can skip the rest of this section.
  • Yes. Make sure to clarify your proposed changes.

Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
@loomlike loomlike added the safe to test Tag to execute build pipeline for a PR from forked repo label Dec 14, 2022
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
enya-yx
enya-yx previously approved these changes Dec 15, 2022
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
Copy link
Collaborator

@Yuqing-cat Yuqing-cat left a comment

Choose a reason for hiding this comment

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

The enhancement looks reasonable. It will be better if we could make _EnvVariableUtil less complex.

feathr_project/feathr/utils/_envvariableutil.py Outdated Show resolved Hide resolved
feathr_project/feathr/utils/_envvariableutil.py Outdated Show resolved Hide resolved
@blrchen
Copy link
Collaborator

blrchen commented Dec 15, 2022

Hello @loomlike seems this doc should get updated as well https://feathr-ai.github.io/feathr/how-to-guides/feathr-configuration-and-env.html#configuration-and-environment-variables-in-feathr

Can I ask is there any use case for introducing this new env? Have this new flag can make configuration more flexible but it introduces complexity on configuration code as well.

@loomlike
Copy link
Collaborator Author

loomlike commented Dec 15, 2022

Hello @loomlike seems this doc should get updated as well https://feathr-ai.github.io/feathr/how-to-guides/feathr-configuration-and-env.html#configuration-and-environment-variables-in-feathr

Can I ask is there any use case for introducing this new env? Have this new flag can make configuration more flexible but it introduces complexity on configuration code as well.

The default behavior is to override the config file values (use_env_vars=True) with env variables, so I think we can keep the document as it is.
Usecases are mostly for us to test with different configuration of the client, since if we change the env var of the test machine for each test case, it's not safe (someone can forget to put the original env var back at the end of the test) as well as parallel testing will not work as they'll share the env vars.

Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
Copy link
Collaborator

@Yuqing-cat Yuqing-cat left a comment

Choose a reason for hiding this comment

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

Looks nice to me! Thanks for the changes.
Please also change the file names envvariableutil.py & test_envvariableutil.py to fit the new class name.

Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
Copy link
Collaborator

@jainr jainr left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes and addressing the comments.

@blrchen blrchen merged commit e53ce96 into feathr-ai:main Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test Tag to execute build pipeline for a PR from forked repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] Add an option to use config file over env vars.
5 participants