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

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 join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants