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

Release fix #1535

Merged
merged 2 commits into from
May 27, 2022
Merged

Release fix #1535

merged 2 commits into from
May 27, 2022

Conversation

davidslater
Copy link
Contributor

Fixes #1531

This updates the build script to not require pip installation. We already require it to be run from the root of the armory repo, so it seems weird to require a pip install to simply import armory.

@lcadalzo
Copy link
Contributor

lcadalzo commented May 27, 2022

lgtm, just one minor comment unrelated to this PR: we currently allow doing either a single build or all the builds. If we were to add nargs='+' to the parser.add_argument() call, this would allow for an arbitary number e.g.

python docker/build.py pytorch tf2

to build the tf2 and pytorch images. If we make this change, we'd need to modify the code below

else:
    frameworks = [args.framework]

since argparse would then already parse the input(s) as a list

@lcadalzo lcadalzo self-assigned this May 27, 2022
@davidslater
Copy link
Contributor Author

@lcadalzo regarding the nargs comment, I think maybe we can revisit that if we expand the number of docker images we create. However, for now, I would like to reserve the possibility of adding more non-keyword args without having to remove the list functionality.

@lcadalzo lcadalzo merged commit b1a923a into twosixlabs:develop May 27, 2022
@davidslater davidslater deleted the release-fix branch May 27, 2022 20:45
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