CmampTask12104_Improve_release_flow_of_prod_image_for_runnable_dir #669
Conversation
Pre-commit checks: - 'check_master' passed - 'check_author' passed - 'check_file_size' passed - 'check_python_compile' passed - 'check_gitleaks' passed All checks passed ✅
Pre-commit checks: - 'check_master' passed - 'check_author' passed - 'check_file_size' passed - 'check_python_compile' passed - 'check_gitleaks' passed All checks passed ✅
|
FYI @PomazkinG |
…d_image_for_runnable_dir
helpers/lib_tasks_docker_release.py
Outdated
| # to ensure the setup in the `prod` image mirrors that of the `dev` image. | ||
| # Use `find_immediate_git_root` instead of `get_git_root` to allow building | ||
| # of `prod` image from submodule. | ||
| git_root_dir = hgit.find_immediate_git_root() |
There was a problem hiding this comment.
I guess we are making the assumptions that 1) runnable dirs are not nested and 2) are only under the git root?
I agree with 1) (although we should document it, since it was implicit), but not sure about 2).
We can have a situation like:
- itsavvy (super-repo)
- cmamp
- helpers (sub-repo)
- app (sub-repo)
In other words, I would call find_git_root and then descend to app doing os.path.relpath to build
Does it make (any) sense? I am a bit confused
There was a problem hiding this comment.
Oh the runnable dir can be nested and doesn't have to be directly under the git root. But we assume that a runnable dir is in a Git repo.
hgit.find_immediate_git_root()is for determining which parent repo to copy (since we want to copy the whole repo). For example, running it on//cmamp//helpers/openai(for openai runnable dir) would return //helpers root//orange//amp/ck_web_app/itsavvy(for itsavvy runnable dir) would return //amp root
- If we use
hgit.find_git_root(), it will return the outermost repo every time. For example,- If we want to build
helpers, but we are in the submodule//cmamp/helpers, it will copy//cmamp, and not just//helpers - If we want to build
itsavvy, but we are in the submodule//orange/amp/ck_web_app/itsavvy, it will copy//orange, and not just//cmamp
- If we want to build
- This is not a problem if we build from the super-module every time
Maybe this naming find_immediate_git_root is confusing? It supposes to return the parent repo (not the outermost) of the current runnable dir that we are trying to build
There was a problem hiding this comment.
I have renamed it to find_parent_repo and added the explanation to make it clearer
There was a problem hiding this comment.
Good explanation (yes find_parent_repo is much better).
What is still confusing is that I would expect that to release itsavvy in //orange/amp/ck_web_app/itsavvy, shouldn't the build context be //orange? Otherwise how to we guarantee that //helpers (and other potentially useful code) is covered?
Maybe I'm being too simplistic, but I would always copy all the code from the outermost repo, and have the outermost repo as build context, and build from the runnable dir.
There was a problem hiding this comment.
What is still confusing is that I would expect that to release itsavvy in //orange/amp/ck_web_app/itsavvy, shouldn't the build context be //orange? Otherwise how to we guarantee that //helpers (and other potentially useful code) is covered?
I'm thinking because itsavvy is part of //cmamp (which is the parent repo), //cmamp build context should be sufficient.
Maybe I'm being too simplistic, but I would always copy all the code from the outermost repo, and have the outermost repo as build context, and build from the runnable dir.
I see. That would be much simpler. I was trying to make the image the same regardless of where it's built. :)
Assuming we want to build helpers, that would mean the prod image that is built from //helpers (which includes only //helpers code) and //cmamp//helpers (which includes //cmamp code) are different, right?
…d_image_for_runnable_dir
Task https://github.com/causify-ai/cmamp/issues/12104