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

--no-docker_pull prevents looking at remote repositories. #502

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tfboyd
Copy link
Member

@tfboyd tfboyd commented Oct 30, 2020

This is useful if the base image has been created locally. Looking for a nonexistent image in a remote repo results in an error.

For tf-agents we build a base image and then build our mujoco image on top of that so users that don't need mujoco do not have to deal with the licensing issues.

If you want the flag to work differently, just let me know. I should have used absl flags when we created this the first time, but I am not sure I was as aware of the advantages. I knew I did not want to use tf.FLAGS. Oh well. Let me know.

This is useful if the base image has been created locally.
Looking for a non-existant image in a remote repo results
in an error.
@tfboyd tfboyd requested a review from sganeshb October 30, 2020 06:51
@google-cla google-cla bot added the cla: yes label Oct 30, 2020
'''
)
parser.add_argument(
'--no-docker_pull',
Copy link
Member

Choose a reason for hiding this comment

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

I think it's a little confusing to create 2 flags for the same bit.

Can the commandline pass --docker_pull="false" or equivalent to get the same effect?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about this? @sganeshb I think this is much better. I over did it the first time. It was getting late.

That is the downside to argparse you cannot do a boolean like you would with absl. It works fine if you want to only have true and default false. Here is another option that gets a very similar result (maybe the same really) but with less lines.

This creates only a --no-docker_pull and no docker_pull (which makes sense as it is True by default). --no-docker_pull sets docker_pull True by default and then flips to false when --no-docker_pull is used.

Note: that if you pass --docker_pull on the commandline it will fail because it is not a flag, which I think is fine.

  parser.add_argument(
      '--no-docker_pull',
      action='store_false',
      default=True,
      dest='docker_pull',
      help='''Set to skip checking remote repositories for updated images. This
      is useful if the base image has been created locally.
      '''
      )

Copy link
Member

Choose a reason for hiding this comment

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

Thanks - that looks better. Should the default be False to preserve old behavior?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants