HIP-infrastructure / tvb

Apache License 2.0
0 stars 0 forks source link

Issues before move to prod #4

Open maedoc opened 2 years ago

maedoc commented 2 years ago

I have a few issues I'd like to resolve in the current iteration of this app on HIP dev prior to seeing it on prod:

non-blocking

iDmple commented 2 years ago

Let me know if I can help on any of these points!

maedoc commented 2 years ago

just this one:

Shutting the app down via menu does not seem to inform HIP that the app is done

when I close the TVB app (via the X in the upper right hand corner), the application list shows the app still running:

image

I would guess that node has not exited even though electron has? I will try to reproduce locally but if you can check why the health check still sees the app as running, it could be helpful.

maedoc commented 2 years ago

@iDmple can you redeploy with latest? I'm now able to run some of the code so I'd like to adapt the filenames to fit what's in the Nextcloud.

maedoc commented 1 year ago

@iDmple can deploy to qa please? I would just like to have a terminal open in the app so I can run some of the scripts.

iDmple commented 1 year ago

First I have to deploy to dev. I will let you know when that's done. I think I'm ready to deploy to qa after, nothing is blocking on my side. If you want to have a terminal you'll have to follow https://github.com/HIP-infrastructure/app-template#dockerfile-modifications. In point 1 use ${DOCKERFS_TYPE}:${DOCKERFS_VERSION}, in point 4 use terminal. dcm2niix spawns a terminal, you can use it as an example.

Edit: looks like you already added the terminal :)

maedoc commented 1 year ago

I don't have access to dev (some oidc denial, I requested access but no response). I'd really like to just get the image running on prod asap so I can finish tutorial notebooks, even if it's just a terminal in the first iteration.

iDmple commented 1 year ago

The build failed with this error:

Step 22/32 : ADD ./apps/${APP_NAME}/license.txt /apps/${APP_NAME}/freesurfer/license.txt
ADD failed: file not found in build context or excluded by .dockerignore: stat apps/tvb/license.txt: file does not exist

To avoid rebuilding apps that are already on the HIP, you can just include them in yours, for example like I did here: https://github.com/HIP-infrastructure/bidsmanager/blob/455530c83aed60dab06acc08b865aad2727cbeef/Dockerfile#L5-L6

maedoc commented 1 year ago

stat apps/tvb/license.txt: file does not exist

What is app name then? I am testing the dockerfile locally with test_dockerfile.sh where it's set to tvb, so the git repo has to put the license file in that folder.

avoid rebuilding apps that are already on the HIP

that works for you because you have access to all those images.. I will see if I can build them myself since avoiding fs and fsl builds would be a significant improvement.

iDmple commented 1 year ago

$APP_NAME is set to tvb, that's correct.

However . doesn't refer to the root of this repo, since the build context is different on the HIP. ./apps/${APP_NAME}/ refers to the root of the tvb repo, and that's where it's searching for the license file, and the other files that you're adding with the ADD command.

If you don't want to move them to the root, you'll have to add them in ./apps/${APP_NAME}/apps/tvb/.

If you look a few commits back, you moved files into the subdirectory apps/tvb, that's what is causing the issue current problems.

maedoc commented 1 year ago

If I move back to root, the ADD directive has to change too. I don't have a preference other than that it works, so I'll try ./apps/${APP_NAME}/apps/tvb next.

you moved files into the subdirectory apps/tvb

just to adapt to the changes made the dockerfile in this commit https://github.com/HIP-infrastructure/tvb/commit/144781616388a389acdefae46b65eaa3ca48fe54 b/c I could no longer build the image myself

iDmple commented 1 year ago

You could add the files to root, and then a symlink to /apps/tvb, I guess that can work for both of us. Otherwise, you can change the build context on your side as well. That way, you can build all of the images you'd like in an easier way.

iDmple commented 1 year ago

TVB is ready on dev for testing @maedoc

maedoc commented 1 year ago

I'm missing some shared libs for GUI apps. I'll debug it locally to see what apt packages to include.

iDmple commented 1 year ago

Okay, the base image is ubuntu:20.04 if that can help.

maedoc commented 1 year ago

thx I'm using the same. I've committed a few changes to add some missing deps. Please build & deploy to dev when you have a chance. If it works like locally, then it should be more or less done.

It may be useful to share the test_dockerfile.sh with other projects, if helpful.

maedoc commented 1 year ago

I just pushed some last changes. I didn't reorder the dockerfile directives yet, so that your build time would be minimal. Obviously the apt lines can be merged later, but otherwise, everything is there and working as far as I can tell through my local setup. Let me know when it's ready to look at on the dev instance plz.