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

Silent errors when docker building #144

Open
AlvaroAguilera opened this issue Oct 7, 2021 · 12 comments
Open

Silent errors when docker building #144

AlvaroAguilera opened this issue Oct 7, 2021 · 12 comments

Comments

@AlvaroAguilera
Copy link

I found another little but very annoying issue.

Compiling an instance with a command like this works perfectly well:

docker run --name build01 -v /data/exp-tasks:/data/ quay.io/vanessa/expfactory-builder build /data/test1 /data/test2 /data/test3/ /data/test4

Except that test3 is not added due to the trailing slash. Since there is no error output, you assume that everything is ok but one or several of the tests is missing....

@vsoch
Copy link
Member

vsoch commented Oct 7, 2021

Do you want to suggest a change / check to handle the trailing slash? If it's just an issue of the slash it should be fairly straight forward.

I suspect that you could add some additional printing here https://github.com/expfactory/expfactory/blob/master/expfactory/cli/build.py#L84-L110 to understand why it's not being added. It could be a simple matter that os.path.exists(experiment) is returning false.

I would suggest to run this container interactively with the same binds but a bash entrypoint, and then pip install ipython, and then insert this right before that loop:

import IPython
IPython.embed()

And then run that expfactory build command with the same arguments. You can then interactively step through that loop and see why it's not being added, and 1. add more verbosity to tell the user, and 2. if you think it's appropriate, fail (sys.exit with a message) if something is given that isn't found. How does that sound?

@vsoch
Copy link
Member

vsoch commented Oct 7, 2021

Feel free to share output / questions you have on here with me. When we find a good fix we can PR to update with it.

@vsoch
Copy link
Member

vsoch commented Sep 27, 2022

Hey if you can give me a repo / example to reproduce this, I can take a look for you. It should be fairly easy to check for (and remove) trailing slashes.

@AlvaroAguilera
Copy link
Author

I guess you can check that with any local test of yours. I don't know if I'm allowed to make our tests public with the studies still going...

@vsoch
Copy link
Member

vsoch commented Sep 28, 2022

Can you show me a dummy example for a Dockerfile, even if you can't share any actual experiment? I just need something to reproduce. Imagine locally cloning the test-task, for example.

@AlvaroAguilera
Copy link
Author

Now that I paid more attention to the messages, when I run this command:

docker run --name build_xxx -v /data/xxx/exp-tasks:/data/ quay.io/vanessa/expfactory-builder build /data/swm_part1_inv /data/swm_part2_inv/ /data/swm_part3_inv

I get the following output:


Expfactory Version: 3.2.12
local experiment /data/swm_part1_inv found, validating...
local experiment /data/swm_part2_inv/ found, validating...
: exp_id parameter swm_part2_inv does not match folder name.
local experiment /data/swm_part3_inv found, validating...
LOG Recipe written to /data/Dockerfile
WARNING 2 local installs detected: build is not reproducible without experiment folders
WARNING not in library, check spelling & punctuation.

To build, cd to directory with Dockerfile and:
docker build --no-cache -t expfactory/experiments .

The exp_id check is considering the slash as part of the id...

@vsoch
Copy link
Member

vsoch commented Sep 29, 2022

well in this case, the exp_id in the config.json doesn't match either of swm_part2_inv or swm_part3_inv. I don't think it's the slash here.

@AlvaroAguilera
Copy link
Author

The id in the config file for the 2nd experiment is defined as follows:

exp_id = "swm_part2_inv"

This id is compared with the folder name given in the docker command without removing the trailing slash:

"swm_part2_inv" == "swm_part2_inv/"

and that fails.

@vsoch
Copy link
Member

vsoch commented Sep 30, 2022

Gotcha! This should be fairly easy to fix - worst case will get in a PR this weekend.

@vsoch
Copy link
Member

vsoch commented Sep 30, 2022

@AlvaroAguilera I added the fixes to the PR here: 74844a3 You should be able to use the examples/docker that is added for that example (and use your same example here that broke it) to test. If we need the builder to be deployed, it might make sense to get that PR merged and then test with the build command directly.

@AlvaroAguilera
Copy link
Author

I'm not sure how to test this in its current state. I can do it as soon as it's merged though.

@vsoch
Copy link
Member

vsoch commented Oct 4, 2022

Alright - merged - give it a bit of time to build and deploy the container (or just check the push timestamp on Quay!)

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

No branches or pull requests

2 participants