Skip to content

Isolate docker build environments #65

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

Merged
merged 14 commits into from
Nov 13, 2022

Conversation

ImogenBits
Copy link
Collaborator

this addresses #64 by isolating the build environments of each image from each other. To do this I archive the newly created image to disk. This comes at a significant performance hit so I also introduced the --unsafe_build CLI flag to have quicker local debugging runs.

logger.error("Removing team {} as their containers did not build successfully.".format(team.name))
self.team_names.remove(team.name)
if command[-2].startswith("generator") and not unsafe_build:
image_archives.pop().unlink()
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand it correctly, we may raise an IndexError here if the very first generator that is being built is not built successfully, as image_archives is still empty at this point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image_archives doesn't just contain the paths to generator archives but also solvers, since they are being built first there never will be a generator that is being built first. the intention is to clean up the solver of the team whose generator failed, not to remove another generator.

@Benezivas
Copy link
Collaborator

Thank you for the pull request, could you please look into the possible exception I commented on and reduce the complexity of the _build method slightly such that the checks run through? I think good candidates for pruning are the first two checks of the method regarding the list of teams.

@ImogenBits
Copy link
Collaborator Author

idk why github isnt running the checks but the linting issues are gone

@Benezivas Benezivas self-assigned this Nov 13, 2022
@Benezivas
Copy link
Collaborator

I had to remove some type hinting as python 3.6 does not implement all of them yet. I like the new option of an --unsafe_build.

Testing shows that we indeed have a performance hit during execution, but the --unsafe_build mitigates at least a little bit of that.
Maybe we can think about optionally loading prebuilt images from users instead of building from directories for version 4.0.0. Since we are now storing and loading images anyway during setup, this change should not require a lot of additional effort. This should also speed up local development by quite a margin, since users can then use dockers caching strategy in order to avoid rebuilding possibly expensive initial setup steps.

Thank you for your work. I will publish this PR together with the encoding fix as a new Subversion.

@Benezivas Benezivas merged commit 3807d67 into Algorithmic-Battle:main Nov 13, 2022
@ImogenBits ImogenBits deleted the local_docker branch November 13, 2022 20:36
@ImogenBits
Copy link
Collaborator Author

Maybe we can think about optionally loading prebuilt images from users instead of building from directories for version 4.0.0.

we essentially already do that, if there already is an image build from that file in the cache docker won't actually build a new one, without the new safe build the battle startup time is pretty marginal already.

@ImogenBits ImogenBits mentioned this pull request Nov 15, 2022
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