Skip to content

Conversation

bbatha
Copy link
Contributor

@bbatha bbatha commented Jun 17, 2020

No description provided.

Copy link
Contributor

@BartoszCki BartoszCki left a comment

Choose a reason for hiding this comment

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

Add some basic test, please.

You could add this option to COMMAND_WITH_ALL_OPTIONS and just add a field to EXPECTED_REQUEST_JSON_WITH_ALL_OPTIONS

@bbatha bbatha requested a review from BartoszCki June 24, 2020 13:58
@bbatha
Copy link
Contributor Author

bbatha commented Jun 24, 2020

@BartoszCki tests added

BartoszCki
BartoszCki previously approved these changes Jul 2, 2020
@bbatha
Copy link
Contributor Author

bbatha commented Jul 6, 2020

@BartoszCki I rebased on master, however now the test for loading from yaml is failing. I'm not sure why and if you have any advice I'd appreciate it. Some more detail:

  1. The fields appear to load from the yaml file in to the raw dict inspected from cli/common
  2. They load at the command line
  3. This was working before I rebased but there appear to be no changes to yaml loading since this pr was originally created.

"--environment",
"environment",
type=json_string,
help="Environmental variables",
Copy link
Contributor

Choose a reason for hiding this comment

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

The "environment" variable is not loaded from yaml file because you have to use
cls=common.GradientOption,
here. Loading values happens in the GradientOption method.Eevery option has to have the cls set to GradientOption for the yaml loading to work. It's not s perfect solution as one have to remember to put it everywhere but I couldn't find any better at the time I was working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I must have lost that in the merge. Thanks! I thought I was going nuts.

@bbatha bbatha requested a review from BartoszCki July 7, 2020 13:42
@bbatha bbatha merged commit 6dd823e into master Jul 10, 2020
@bbatha bbatha deleted the feat-notebook-environment branch July 10, 2020 14:24
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.

2 participants