Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Add support for copy_file tag #467

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.

Sign up for GitHub

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

Jump to bottom
Merged
mark-petersen merged 1 commit into from
Mar 11, 2020
Merged

Add support for copy_file tag #467

mark-petersen merged 1 commit into from
Mar 11, 2020

Conversation

@xylar
Copy link
Collaborator

@xylar xylar commented Mar 6, 2020

This is needed for copying config files that may then be altered by the user within a test case.

Since there is a significant amount of redundant code between this and and support for the add_link tag, a helper function has been added to encapsulate the redundancy.

Some error messages have been modified to raise exceptions.

@xylar
Add support for copy_file tag
2244845 
This is needed for copying config files that may then be altered
by the user within a test case.

Since there is a significant amount of redundant code between this
and and support for the `add_link` tag, a helper function has been
added to encapsulate the redundancy.

Some error messages have been modified to raise exceptions.
@xylar
Copy link
Collaborator Author

xylar commented Mar 6, 2020

@mark-petersen, let me know if develop is not where this belongs. I think this is what we've done in the past with regard to changing the shared COMPASS infrastructure.

@xylar
Copy link
Collaborator Author

xylar commented Mar 6, 2020

@matthewhoffman, #417 merged in @mark-petersen's work in progress on making initial conditions, mapping files, etc. that are ready to go into E3SM. One of the things we realized we couldn't do in COMPASS was copy (as opposed to symlink) a file. In this particular case, we want to copy a config file that a user would then alter as needed for a particular config (e.g for debugging).

@xylar
Copy link
Collaborator Author

xylar commented Mar 6, 2020 •
edited
Loading

Testing

I cherry-picked this commit onto ocean/develp and then built the QU240 init and test stages, and ran all steps of the init stage. All the symlinks are good and the file_copy step worked as expected.

I also set up all of the other global_ocean test cases. The only broken symlinks were the expected ones (because I didn't run the test cases) that point to other steps and stages of the test cases themselves.

It would be good to setup and run as many test cases as practical before merging this, since the add_link code has a lot of if statements that might not be tested in a typical setup.

@xylar xylar mentioned this pull request Mar 7, 2020
Merged
mark-petersen
mark-petersen approved these changes Mar 11, 2020
View reviewed changes
Copy link
Contributor

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

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

@xylar I made a similar change locally, but then forgot to merge it into develop. Thanks for your willingness to pitch in, as always. I also tested with this merged into ocean/develop and it worked just right.

@mark-petersen mark-petersen removed the request for review from matthewhoffman March 11, 2020 13:21
@mark-petersen
Copy link
Contributor

mark-petersen commented Mar 11, 2020

@mgduda this goes into develop, but I'm not asking for your review because it is only in COMPASS.

@matthewhoffman, this is adding a new COMPASS function copy_file as only add_link was there before. This is really useful for config files where it is useful to edit locally, but you don't want to alter a file linked back to the repo.

@mark-petersen mark-petersen merged commit eff73b1 into MPAS-Dev:develop Mar 11, 2020
@xylar xylar deleted the compass/add_copy_file branch March 11, 2020 13:29
@xylar
Copy link
Collaborator Author

xylar commented Mar 11, 2020

@matthewhoffman, I know this is already merged, but could you run a test case or 2 from landice to make sure add_link still works as before for you? I did quite a bit of testing on the ocean side so I'm not worried but it would be better to know sooner rather than later if there's a problem.

mark-petersen added a commit that referenced this pull request Mar 11, 2020
@mark-petersen
Merge PR #467 'xylar/compass/add_copy_file' into ocean/develop
4c0248f 
Note: This was originally merged into develop. Adding it to
ocean/develop so we have it now.

This is needed for copying config files that may then be altered by the
user within a test case.

Since there is a significant amount of redundant code between this and
and support for the add_link tag, a helper function has been added to
encapsulate the redundancy.

Some error messages have been modified to raise exceptions.
@mark-petersen
Copy link
Contributor

mark-petersen commented Mar 11, 2020

@xylar, I added this one commit to ocean/develop so we have it now. I did a local test merge of ocean/develop into develop and there are no conflicts, as the setup_testcase.py file is identical on both branches.

@matthewhoffman
Copy link
Member

matthewhoffman commented Mar 11, 2020

@xylar , I'll try to double check this today when I'm doing some COMPASS stuff. This is a valuable feature that I've wanted in the past. Thanks for putting it together.

@xylar
Copy link
Collaborator Author

xylar commented Mar 11, 2020

I added this one commit to ocean/develop so we have it now. I did a local test merge of ocean/develop into develop and there are no conflicts, as the setup_testcase.py file is identical on both branches.

In this case, I'm okay with this. In general, we don't want to put in redundant commits because it makes debugging a lot more challenging because the same change is effectively introduced in two places.

@mark-petersen
Copy link
Contributor

mark-petersen commented Mar 11, 2020

I agree. The alternative today was to merge develop back to all three cores at once and make a separate E3SM PR for just that, which is way too much overhead for this small change on a lone file in COMPASS.

@matthewhoffman
Copy link
Member

matthewhoffman commented Mar 16, 2020

@xylar , I included this in a COMPASS run today, and everything worked. Nice work.

ashwathsv pushed a commit to ashwathsv/MPAS-Model that referenced this pull request Jul 21, 2020
@mark-petersen
Merge PR MPAS-Dev#467 'xylar/compass/add_copy_file' into ocean/develop
e3e9430 
Note: This was originally merged into develop. Adding it to
ocean/develop so we have it now.

This is needed for copying config files that may then be altered by the
user within a test case.

Since there is a significant amount of redundant code between this and
and support for the add_link tag, a helper function has been added to
encapsulate the redundancy.

Some error messages have been modified to raise exceptions.
@mark-petersen mark-petersen mentioned this pull request Jul 23, 2020
Merged
jonbob added a commit to E3SM-Project/E3SM that referenced this pull request Jul 27, 2020
@jonbob
Merge branch 'mark-petersen/mpas/framework_update_200723' into next (PR
a7a1c71 
#3737)

Update MPAS framework

This PR brings in a new mpas-source submodule with updates to the mpas framework
itself, plus changes to the cores supporting the framework update. Some changes
are made to the atmosphere core, even though it is not used by E3SM, in order to
maintain consistency with the framework. This update includes the following
individual branches and PRs, many of which are additions to the makefiles for
summit, or optional libraries:
* az/azamat/mpas-cmake/mpas-tool-dir  (MPAS-Dev/MPAS-Model#629)
* init_atmosphere/kd_tree_ties  (MPAS-Dev/MPAS-Model#630)
* mark-petersen/framework/add_FillValue  (MPAS-Dev/MPAS-Model#616)
* mark-petersen/framework/add_lapack_option  (MPAS-Dev/MPAS-Model#613)
* amametjanov/framework/nullify-field-pointers  (MPAS-Dev/MPAS-Model#578)
* framework/makefile_e3sm  (MPAS-Dev/MPAS-Model#603)
* xylar/framework/make_shell_bash  (MPAS-Dev/MPAS-Model#594)
* registry/missing_value  (MPAS-Dev/MPAS-Model#562)
* pwolfram/updates_make_intel_stack  (MPAS-Dev/MPAS-Model#592)
* azamat/framework/pgi-cpr-omp-workaround  (MPAS-Dev/MPAS-Model#449)
* az/framework/e3sm-cmake-qnosmp-typo  (MPAS-Dev/MPAS-Model#579)
* xylar/compass/add_prerequisite_tag
* atmosphere/fix_timekeeping_imports (MPAS-Dev/MPAS-Model#582)
* xylar/fix_docs_ci  (MPAS-Dev/MPAS-Model#575)
* xylar/add_compass_docs  (MPAS-Dev/MPAS-Model#472)
* philipwjones/framework/summitmake  (MPAS-Dev/MPAS-Model#536)
* atmosphere/atm_core_cleanup  (MPAS-Dev/MPAS-Model#548)
* init_atmosphere/parse_geoindex  (MPAS-Dev/MPAS-Model#459)
* xylar/compass/add_copy_file  (MPAS-Dev/MPAS-Model#467)
* init_atmosphere/kd_tree  (MPAS-Dev/MPAS-Model#438)
* init_atmosphere/mpas_stack  (MPAS-Dev/MPAS-Model#426)
* operators/add_mpas_in_cell  (MPAS-Dev/MPAS-Model#400)

Fixes #3396
Fixes #3236

[BFB]
jonbob added a commit to E3SM-Project/E3SM that referenced this pull request Jul 29, 2020
@jonbob
Merge branch 'mark-petersen/mpas/framework_update_200723' (PR #3737)
ffe5c28 
Update MPAS framework

This PR brings in a new mpas-source submodule with updates to the mpas framework
itself, plus changes to the cores supporting the framework update. Some changes
are made to the atmosphere core, even though it is not used by E3SM, in order to
maintain consistency with the framework. This update includes the following
individual branches and PRs, many of which are additions to the makefiles for
summit, or optional libraries:
* az/azamat/mpas-cmake/mpas-tool-dir  (MPAS-Dev/MPAS-Model#629)
* init_atmosphere/kd_tree_ties  (MPAS-Dev/MPAS-Model#630)
* mark-petersen/framework/add_FillValue  (MPAS-Dev/MPAS-Model#616)
* mark-petersen/framework/add_lapack_option  (MPAS-Dev/MPAS-Model#613)
* amametjanov/framework/nullify-field-pointers  (MPAS-Dev/MPAS-Model#578)
* framework/makefile_e3sm  (MPAS-Dev/MPAS-Model#603)
* xylar/framework/make_shell_bash  (MPAS-Dev/MPAS-Model#594)
* registry/missing_value  (MPAS-Dev/MPAS-Model#562)
* pwolfram/updates_make_intel_stack  (MPAS-Dev/MPAS-Model#592)
* azamat/framework/pgi-cpr-omp-workaround  (MPAS-Dev/MPAS-Model#449)
* az/framework/e3sm-cmake-qnosmp-typo  (MPAS-Dev/MPAS-Model#579)
* xylar/compass/add_prerequisite_tag
* atmosphere/fix_timekeeping_imports (MPAS-Dev/MPAS-Model#582)
* xylar/fix_docs_ci  (MPAS-Dev/MPAS-Model#575)
* xylar/add_compass_docs  (MPAS-Dev/MPAS-Model#472)
* philipwjones/framework/summitmake  (MPAS-Dev/MPAS-Model#536)
* atmosphere/atm_core_cleanup  (MPAS-Dev/MPAS-Model#548)
* init_atmosphere/parse_geoindex  (MPAS-Dev/MPAS-Model#459)
* xylar/compass/add_copy_file  (MPAS-Dev/MPAS-Model#467)
* init_atmosphere/kd_tree  (MPAS-Dev/MPAS-Model#438)
* init_atmosphere/mpas_stack  (MPAS-Dev/MPAS-Model#426)
* operators/add_mpas_in_cell  (MPAS-Dev/MPAS-Model#400)

Fixes #3396
Fixes #3236

[BFB]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mark-petersen mark-petersen mark-petersen approved these changes

Assignees

@mark-petersen mark-petersen

Labels

COMPASS feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@xylar @mark-petersen @matthewhoffman
Morty Proxy This is a proxified and sanitized view of the page, visit original site.