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

Fix/git detached head #736

Merged
merged 11 commits into from
Aug 5, 2022
Merged

Fix/git detached head #736

merged 11 commits into from
Aug 5, 2022

Conversation

pgierz
Copy link
Member

@pgierz pgierz commented Jul 12, 2022

Should fix the broken git branch stuff with detached head, sorry about that.

@pgierz pgierz requested review from denizural and mandresm July 12, 2022 15:13
@pgierz pgierz added the bug Something isn't working label Jul 12, 2022
@pgierz pgierz marked this pull request as ready for review July 12, 2022 15:13
@pgierz
Copy link
Member Author

pgierz commented Jul 12, 2022

There is probably a better way to do this, but my brain is fried today. Someone please double check.

@mandresm
Copy link
Contributor

Some models are still broken in the tests. I'll review this tomorrow and maybe even have a look at the models having problems (AWICM3)

@pgierz
Copy link
Member Author

pgierz commented Jul 13, 2022

As with the doc-test branch, all failing tests are now from esm-tests, and all seem to be "Run Files Differ"

@mandresm
Copy link
Contributor

#bump

@pgierz
Copy link
Member Author

pgierz commented Jul 13, 2022

Will close #734, still needs some manual testing

@pgierz pgierz linked an issue Jul 13, 2022 that may be closed by this pull request
@pgierz
Copy link
Member Author

pgierz commented Jul 13, 2022

Manually tested with AWICM3 frontiers-xios and AWICM3 3.1.

@JanStreffing: something is still weird. But if you are happy with this, then that is how it shall be. Currently xios does not seem to have a model directory, and rnfmap is not a git-controlled thing.:

$ cat ../experiments/git_test_001/run_20000101-20011231/log/git_test_001_vcs_info.yaml
esm_tools: d3e90a56
fesom:
  branch_name: awicm-3-frontiers_parallel-restart
  diffs:
  - ''
  hash: 9b27593c
  path: /work/ab0995/a270077/SciComp/Model_Support/AWICM3/broken_git_check/model_codes/awicm3-frontiers-xios///fesom-2.0
oasis3mct:
  branch_name: awicm-3-frontiers
  diffs:
  - ''
  hash: 347e788
  path: /work/ab0995/a270077/SciComp/Model_Support/AWICM3/broken_git_check/model_codes/awicm3-frontiers-xios///oasis
oifs:
  branch_name: awicm-3-frontiers-xios
  diffs:
  - diff --git a/src/ifs/module/yommcc.F90 b/src/ifs/module/yommcc.F90
  - index e7d74a1..131227a 100644
  - '--- a/src/ifs/module/yommcc.F90'
  - +++ b/src/ifs/module/yommcc.F90
  - '@@ -180,7 +180,7 @@ INTEGER(KIND=JPIM) :: NFRCPL'
  - ' '
  - ' !Logical switches for Coupling'
  - ' LOGICAL :: COUPLENEMOECE = .FALSE.   ! Use EC-Earth NEMO coupling'
  - '-LOGICAL :: COUPLEFESOM2 = .FALSE.    ! Use AWI FESOM2 coupling'
  - '+LOGICAL :: COUPLEFESOM2 = .TRUE.    ! Use AWI FESOM2 coupling'
  - ' LOGICAL :: COUPLENEMOFOCI = .FALSE.   ! Use FOCI NEMO coupling'
  - ' LOGICAL :: LNEMOLIMGET = .FALSE.   ! Get new fields'
  - ' LOGICAL :: LNEMOLIMPUT = .FALSE.   ! Put new fields'
  hash: c78fa46
  path: /work/ab0995/a270077/SciComp/Model_Support/AWICM3/broken_git_check/model_codes/awicm3-frontiers-xios///oifs-43r3
rnfmap: Not a git-controlled model!
xios: Unable to locate model_dir for xios.

...and, yes, I am not particularly happy that diffs is a list of strings. I might still change that.

@JanStreffing
Copy link
Contributor

Not sure why it's saying rnfmap is not a git controlled thing. It certainly is:
https://gitlab.dkrz.de/ec-earth/runoff-mapper
XIOS seems to use the model_dir from awicm3 and install_bins: bin/xios.x
No harm in adding a model_dir for consistencies sake I guess. Though it seems required to run the model.

@pgierz
Copy link
Member Author

pgierz commented Jul 13, 2022

Jan, have a look in levante: /work/ab0995/a270077/SciComp/Model_Support/AWICM3/broken_git_check/model_codes/awicm3-frontiers-xios

It might be that the install_bins part is generating folders for things that aren't git repos (or at least, are not git repos where the model_dir is defined). That's too deep in the AWI-CM3 config for me to be comfortable touching it though...

@pgierz pgierz requested a review from mandresm July 13, 2022 11:25
@mandresm
Copy link
Contributor

mandresm commented Aug 1, 2022

There seems to be a problem in FOCI (blogin):

    File "/github/home/.local/bin/esm_runscripts", line 11, in <module>
      load_entry_point('esm-tools', 'console_scripts', 'esm_runscripts')()
    File "/__w/esm_tools/esm_tools/src/esm_runscripts/cli.py", line 278, in main
      Setup()
    File "/__w/esm_tools/esm_tools/src/esm_runscripts/sim_objects.py", line 69, in __call__
      self.prepcompute()
    File "/__w/esm_tools/esm_tools/src/esm_runscripts/sim_objects.py", line 161, in prepcompute
      self.config = prepcompute.run_job(self.config)
    File "/__w/esm_tools/esm_tools/src/esm_runscripts/prepcompute.py", line 36, in run_job
      config = evaluate(config, "prepcompute", "prepcompute_recipe")
    File "/__w/esm_tools/esm_tools/src/esm_runscripts/helpers.py", line 68, in evaluate
      config = esm_plugin_manager.work_through_recipe(
    File "/__w/esm_tools/esm_tools/src/esm_plugin_manager/esm_plugin_manager.py", line 141, in work_through_recipe
      config = getattr(submodule, workitem)(config)
    File "/__w/esm_tools/esm_tools/src/esm_runscripts/prepare.py", line 707, in add_vcs_info
      if helpers.is_git_repo(model_dir):
    File "/__w/esm_tools/esm_tools/src/esm_runscripts/helpers.py", line 329, in is_git_repo
      git.Repo(path).git_dir
    File "/github/home/.local/lib/python3.8/site-packages/git/repo/base.py", line 152, in __init__
      raise NoSuchPathError(epath)
  git.exc.NoSuchPathError: /__w/esm_tools/esm_tools/src/esm_tests/resources/automatic_testing/comp/focioifs/focioifs-2.0/oifs-43r3
  Copying standard yamls from:  /__w/esm_tools/esm_tools/configs/
  Copying standard namelists from:  /__w/esm_tools/esm_tools/namelists/
  /__w/esm_tools/esm_tools/src/esm_tests/resources/runscripts/focioifs/focioifs-piCtl-test_lowcpu.yaml

Do you want me to try to reproduce it in a real computer?

@pgierz
Copy link
Member Author

pgierz commented Aug 1, 2022

No, that's something else, and actually is still a bug in the git part. I'll look after I do the pool stuff.

@pgierz
Copy link
Member Author

pgierz commented Aug 4, 2022

Broken tests are at this point "wrong truth". The technical elements seem to working fine. The error reported by @mandresm is now fixed. I am not 100% sure if my fix is the best way of doing it.

The offending function checks: is this a git directory? You get True if it is a git dir (makes sense), and you get False if it is not (make sense) OR False if it is non-existent (this part I am not sure about). Throwing the FileNotFound error might be better here, but that kills our tests. That FileNotFound case only happens on CI; so it will never be a problem in real production.

In any case, I would vote: please merge. :-)

@mandresm
Copy link
Contributor

mandresm commented Aug 5, 2022

The offending function checks: is this a git directory? You get True if it is a git dir (makes sense), and you get False if it is not (make sense) OR False if it is non-existent (this part I am not sure about). Throwing the FileNotFound error might be better here, but that kills our tests. That FileNotFound case only happens on CI; so it will never be a problem in real production.

Agreed, this is the sort of thing that is going to be tested in the actual esm_tests, and we have to run those from time to time anyway, so I think it's okay if it doesn't work for check tests.

Copy link
Contributor

@mandresm mandresm left a comment

Choose a reason for hiding this comment

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

Great feature, thanks for fixing it! :)

@mandresm
Copy link
Contributor

mandresm commented Aug 5, 2022

updating the truth...

mandresm added a commit to esm-tools/esm_tests_info that referenced this pull request Aug 5, 2022
mandresm added a commit to esm-tools/esm_tests_info that referenced this pull request Aug 5, 2022
mandresm added a commit to esm-tools/esm_tests_info that referenced this pull request Aug 5, 2022
mandresm added a commit to esm-tools/esm_tests_info that referenced this pull request Aug 5, 2022
mandresm added a commit to esm-tools/esm_tests_info that referenced this pull request Aug 5, 2022
@mandresm
Copy link
Contributor

mandresm commented Aug 5, 2022

All tests passed (except AWIESM icebergs due to lack of update related to an ongoing issue), #bump!

@mandresm mandresm merged commit bf7c910 into release Aug 5, 2022
@mandresm mandresm deleted the fix/git_detached_head branch August 5, 2022 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

git feature #690 broken
3 participants