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

Major progress converting perl to python: case.build and check_input_… #61

Closed
wants to merge 2 commits into from

Conversation

jgfouca
Copy link
Contributor

@jgfouca jgfouca commented Feb 11, 2016

…data

Things to note:

  1. Machines will now need to specific python information in the modules
    section of config_machines.xml
  2. Fix a few mistakes in config_component.xml, errant ">" character
  3. Implemented Case class from perl-OO branch
    -This should replace xmlquery/xmlchange
  4. Had to get rid of XML version in xmlchange. This was causing XML
    files to diff between locked files. This restriction can go away
    once everything is using Case.
  5. New python tools that need to be in caseroot are symlinked instead
    of copied. This makes it so all the python/utils don't have to be copied.
  6. Add classes for all the env*.xml files. They all inherit from a common
    base class to reduce code duplication.
  7. Add new env_module module to replace ModuleLoader.pm

…data

Things to note:
1) Machines will now need to specific python information in the modules
section of config_machines.xml
2) Fix a few mistakes in config_component.xml, errant ">" character
3) Implemented Case class from perl-OO branch
  * This should replace xmlquery/xmlchange
4) Had to get rid of XML version in xmlchange. This was causing XML
files to diff between locked files. This restriction can go away
once everything is using Case.
5) New python tools that need to be in caseroot are symlinked instead
of copied. This makes it so all the python/utils don't have to be copied.
6) Add classes for all the env*.xml files. They all inherit from a common
base class to reduce code duplication.
7) Add new env_module module to replace ModuleLoader.pm
@jgfouca
Copy link
Contributor Author

jgfouca commented Feb 11, 2016

I will annotate to help you review.

help="Case directory to build")

parser.add_argument("-t", "--testmode", action="store_true",
help="Unknown TODO")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't sure what this option means.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops that was a different option. Some of the tests require multiple model builds, to accommodate that we have a case.test_build script. If you are in a test that requires multiple builds and do not run case.build from case.test_build it will throw an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, added comment.

@jedwards4b
Copy link
Contributor

Review complete, no major concerns.

os.environ["SMP"] = "FALSE"

# TODO - Special case for clm?
# TODO - bldroot?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm trying to understand how bldroot works in this algorithm. It looks like it might be left as empty string in many cases in the perl version. Is this right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes if it's empty the default setting of EXEROOT is used

@quantheory
Copy link
Contributor

I haven't been following this too closely (don't have time), but if you're using Python 2 specifically I recommend using the shebang:

#!/usr/bin/env python2

instead of

#!/usr/bin/env python

There are a few weird cases (e.g. some Linux distributions) where python is soft-linked to python3. So if you're only testing these scripts with Python 2, you probably want to specify that that's what you want.

@bertinia
Copy link
Contributor

In the postprocessing scripts, we actually go a little further with the
following snippet of code at the
top of the script to ensure 2.7.x is being using:

#!/usr/bin/env python

from future import print_function

import sys

check the system python version and require 2.7.x or greater

if sys.hexversion < 0x02070000:
print(70 * "")
print("ERROR: {0} requires python >= 2.7.x. ".format(sys.argv[0]))
print("It appears that you are running python {0}".format(
".".join(str(x) for x in sys.version_info[0:3])))
print(70 * "
")
sys.exit(1)

On Tue, Feb 16, 2016 at 2:17 PM, Sean Patrick Santos <
[email protected]> wrote:

I haven't been following this too closely (don't have time), but if you're
using Python 2 specifically I recommend using the shebang:

#!/usr/bin/env python2

instead of

#!/usr/bin/env python

There are a few weird cases (e.g. some Linux distributions) where python
is soft-linked to python3. So if you're only testing these scripts with
Python 2, you probably want to specify that that's what you want.


Reply to this email directly or view it on GitHub
#61 (comment).

Alice Bertini
NCAR - CSEG Software Engineer

@jedwards4b jedwards4b mentioned this pull request Feb 16, 2016
@jedwards4b
Copy link
Contributor

Superceded by PR #62

@jedwards4b jedwards4b closed this Feb 16, 2016
@rljacob
Copy link
Member

rljacob commented Feb 16, 2016

@quantheory or @bertinia , please make a github issue about verifying python versions so it doesn't get lost in the PR review comments.

@jgfouca
Copy link
Contributor Author

jgfouca commented Feb 17, 2016

@quantheory , python2 is probably better. The one problem, which I immediately noticed, is that emacs was not smart enough to load python-mode after this change was made. It was easy to fix in my .emacs file after some googling, but it might trip up other users.

@jgfouca jgfouca deleted the jgfouca/more_xml branch February 23, 2016 21:21
jayeshkrishna pushed a commit that referenced this pull request Aug 16, 2016
now duplicate MPI communicators in init_intracomm
jgfouca pushed a commit that referenced this pull request Dec 3, 2018
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

Successfully merging this pull request may close these issues.

5 participants