Skip to content
This repository has been archived by the owner on Jan 7, 2025. It is now read-only.

Use envvars for configuration instead of a file #1091

Merged
merged 3 commits into from
Sep 21, 2016

Conversation

lukeyeager
Copy link
Member

@lukeyeager lukeyeager commented Sep 19, 2016

Close #941, close #180, fix #1090
Bug 1804528

Env vars are easy to change between deploys without changing any code; unlike config files, there is little chance of them being checked into the code repo accidentally; and unlike custom config files, or other config mechanisms such as Java System Properties, they are a language- and OS-agnostic standard.
https://12factor.net/config

Also,

  • Once we add a working setup.py, multiple users will be able to start their own customized instance from the same installed codebase (instance-specific config capability needed #941)
  • The ability to set the configuration interactively doesn't scale well as we continue to add options

@gheinrich
Copy link
Contributor

This seems to improve the time it takes to start tools (and this should be reflected on Travis test time too):

before:

$ time ./tools/inference.py --help
real    0m0.821s

After:

$ time ./tools/inference.py --help
real    0m0.438s

Copy link
Contributor

@gheinrich gheinrich left a comment

Choose a reason for hiding this comment

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

Can we document the full list of environment variables?

@classmethod
def get_extensions(cls, show_all):
return extensions.view.get_extensions(show_all=show_all)
if 'DIGITS_ALL_EXTENSIONS' in os.environ:
Copy link
Contributor

Choose a reason for hiding this comment

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

nice simplification!

# make the directory
os.mkdir(self._config_file_value)

if 'DIGITS_MODE_TEST' in os.environ:
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe move this to a function like you are doing for other options like log_file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you really think that's more legible? I thought I'd just keep it simple when I can.

Copy link
Contributor

Choose a reason for hiding this comment

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

No I think it's fine this way, forget that comment.

if config_value('caffe_root')['flavor'] == 'NVIDIA':
if config_value('caffe_root')['version'] > parse_version('0.14.0-alpha'):
if config_value('caffe')['flavor'] == 'NVIDIA':
if parse_version(config_value('caffe')['version']) > parse_version('0.14.0-alpha'):
Copy link
Contributor

Choose a reason for hiding this comment

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

tested this path - ok



if 'CAFFE_ROOT' in os.environ:
executable, version, flavor = load_from_envvar('CAFFE_ROOT')
Copy link
Contributor

Choose a reason for hiding this comment

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

tested this path - OK

raise ValueError('Torch executable not found at "%s" (TORCH_HOME)'
% os.environ['TORCH_HOME'])
else:
executable = find_executable()
Copy link
Contributor

Choose a reason for hiding this comment

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

tested this path - OK

@lukeyeager
Copy link
Member Author

Can we document the full list of environment variables?

I added a doc at docs/Configuration.md.

Copy link
Contributor

@gheinrich gheinrich left a comment

Choose a reason for hiding this comment

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

@lukeyeager lukeyeager mentioned this pull request Sep 20, 2016
5 tasks
@lukeyeager
Copy link
Member Author

Thanks for the review Greg! I'm going to merge this now, to enable #1093 (which enables #927).

@lukeyeager lukeyeager merged commit 3afc367 into NVIDIA:master Sep 21, 2016
@lukeyeager lukeyeager deleted the envvars branch September 21, 2016 16:50
@lukeyeager
Copy link
Member Author

Oops, I meant to squash before merging. Sorry.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants