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

Revamp AWS configuration module #59

Merged
merged 6 commits into from Dec 16, 2019
Merged

Revamp AWS configuration module #59

merged 6 commits into from Dec 16, 2019

Conversation

savingoyal
Copy link
Collaborator

@savingoyal savingoyal commented Dec 16, 2019

Redo metaflow configure aws. Goes hand-in-hand with Netflix/metaflow-tools#3

Copy link
Contributor

@crk-codaio crk-codaio left a comment

Mostly looks good, will do some QA (for backward compat) later but I think it should be fine.

@@ -349,83 +348,79 @@ def sandbox(profile):
# Persist to a file.
persist_env(env_dict, profile)

@configure.command(help='Get Metaflow up and running on your own AWS environment.')
@configure.command(help='Configure Metaflow to access self-managed AWS resources.')
Copy link
Contributor

@crk-codaio crk-codaio Dec 16, 2019

Choose a reason for hiding this comment

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

Any reason you chose against the 'Get Metaflow up and running on your self-managed AWS resources.'? Is it getting trimmed?

Currently the language looks slightly inconsistent between sandbox and aws:

Commands:
  aws      Configure Metaflow to access self-managed AWS resources.
  reset    Reset configuration to run locally.
  sandbox  Get Metaflow up and running on our sandbox.
  show     Show existing configuration.

Copy link
Collaborator Author

@savingoyal savingoyal Dec 16, 2019

Choose a reason for hiding this comment

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

Given that all the statements start with an assertive verb, it made sense to modify Get... to Configure.... I will make a similar language change for sandbox.

"configurations. Please `export METAFLOW_PROFILE` to "
"switch between profile configuration(s).")
help='Configure a named profile. Activate the profile by setting '
'`METAFLOW_PROFILE` environment variable.')
Copy link
Contributor

@crk-codaio crk-codaio Dec 16, 2019

Choose a reason for hiding this comment

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

Indentation off by 1 on this line.

Copy link
Collaborator Author

@savingoyal savingoyal Dec 16, 2019

Choose a reason for hiding this comment

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

Will fix.

use_s3 = click.confirm('\nConfigure Amazon '
'S3 as default datastore (required for AWS Batch)?',
default=True, abort=False)
Copy link
Contributor

@crk-codaio crk-codaio Dec 16, 2019

Choose a reason for hiding this comment

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

Can you add 'Amazon S3' in cyan?

Copy link
Collaborator Author

@savingoyal savingoyal Dec 16, 2019

Choose a reason for hiding this comment

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

I tried that, it ended up being too many colors.


env_dict['METAFLOW_DATATOOLS_SYSROOT_S3'] =\
click.prompt('\t' + cyan('METAFLOW_DATATOOLS_SYSROOT_S3:') +
yellow(' (optional)') + ' Amazon S3 url for metaflow datatools (s3://<bucket>/<prefix>)',
Copy link
Contributor

@crk-codaio crk-codaio Dec 16, 2019

Choose a reason for hiding this comment

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

Nit: Do you need this again, since you suggest a default in brackets right after this. I vote for removing it, since it seems pretty clear with the actual default value.

METAFLOW_DATATOOLS_SYSROOT_S3: (optional) Amazon S3 url for metaflow datatools (s3://<bucket>/<prefix>) [s3://foo/metaflow/data]

Copy link
Collaborator Author

@savingoyal savingoyal Dec 16, 2019

Choose a reason for hiding this comment

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

This is to follow a consistent pattern throughout.

use_batch =\
click.confirm('\nDo you want to use AWS Batch for compute?',
click.confirm('\nConfigure AWS Batch for compute?',
Copy link
Contributor

@crk-codaio crk-codaio Dec 16, 2019

Choose a reason for hiding this comment

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

Same for AWS Batch in cyan? The original intent is to given an idea what the next set of configuration is going to be about, with the cyan highlight(s).

Copy link
Collaborator Author

@savingoyal savingoyal Dec 16, 2019

Choose a reason for hiding this comment

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

Similar argument as above.


env_dict['METAFLOW_BATCH_JOB_QUEUE'] =\
click.prompt('\t' + cyan('METAFLOW_BATCH_JOB_QUEUE:') +
' AWS Batch Job Queue')
Copy link
Contributor

@crk-codaio crk-codaio Dec 16, 2019

Choose a reason for hiding this comment

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

Could we be a bit more specific here? I thought there was some confusion what entry to have here (the arn or the queue name or what).

Copy link
Collaborator Author

@savingoyal savingoyal Dec 16, 2019

Choose a reason for hiding this comment

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

Both of them work equally well. For any of the configs, the user can specify ARNs or names as they are interchangeable.

env_dict['METAFLOW_CONDA_PACKAGE_S3ROOT'] = package_s3root
env_dict['METAFLOW_SERVICE_URL'] =\
click.prompt('\t' + cyan('METAFLOW_SERVICE_URL:') +
' URL for Metadata Service (Open to Public Access)')
Copy link
Contributor

@crk-codaio crk-codaio Dec 16, 2019

Choose a reason for hiding this comment

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

Could you double check if sentence case or lower case - which looks better - for the info in ()?

Copy link
Collaborator Author

@savingoyal savingoyal Dec 16, 2019

Choose a reason for hiding this comment

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

I went with the sentence case for continuity here.

package_s3root = \
click.prompt('\tPlease enter the bucket prefix for storing '
'conda packages',
default=default_val)
env_dict['METAFLOW_CONDA_PACKAGE_S3ROOT'] = package_s3root
Copy link
Contributor

@crk-codaio crk-codaio Dec 16, 2019

Choose a reason for hiding this comment

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

Just to double check - have you decided to retire this configuration for METAFLOW_CONDA_PACKAGE_S3ROOT?

Copy link
Collaborator Author

@savingoyal savingoyal Dec 16, 2019

Choose a reason for hiding this comment

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

Yes, indeed.

@savingoyal savingoyal merged commit a9857b7 into master Dec 16, 2019
2 checks passed
@savingoyal savingoyal deleted the aws branch Dec 19, 2019
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.

None yet

2 participants