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.

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

Rational config handling #23

Merged
merged 13 commits into from
May 25, 2018
Merged

Rational config handling #23

merged 13 commits into from
May 25, 2018

Conversation

PDoakORNL
Copy link
Collaborator

To accomplish support for using pycicle with a personal fork and a PR destination branch other than master. I tried to be careful to not break or change any current functionality. For instance a good test would be hpx should be able to run with this as if it were master.

Unfortunately my existing CI instances weren't upgraded to the post merge master which did produce some minor breaking changes to the DCA CI on condo.

@PDoakORNL PDoakORNL mentioned this pull request May 22, 2018
Open
biddisco
biddisco requested changes May 24, 2018
View reviewed changes
Copy link
Owner

@biddisco biddisco left a comment

Choose a reason for hiding this comment

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

the pbs/slurm stuff is the only thing that stands out as being dodgy.

README.md
ssh -fN -R38080:localhost:8080 you@clusterA-login-node
```
If you have load balancing on log in nodes make sure to explicitly raise the reverse tunnel on the login node pycicle is running on.

Copy link
Owner

Choose a reason for hiding this comment

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

Doc improvements Wonderful

config/dca/cadesGPUCondo.cmake Outdated

# Launch jobs using slurm rather than directly running them on the machine
set(PYCICLE_SLURM FALSE)
set(PYCICLE_PBS TRUE)
Copy link
Owner

Choose a reason for hiding this comment

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

I think the set(PYCICLE_JOB_LAUNCH "pbs") approach is nicer than having a bool, for each possiblity. This was changed in my last big update and I suspect you started adding these tweaks before I merged your other stuff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't test the dca master branch CI so I missed this.

config/dca/cadesGPUCondo.cmake Outdated
@@ -110,7 +112,7 @@ string(CONCAT CTEST_BUILD_OPTIONS ${CTEST_BUILD_OPTIONS}
# Setup a slurm job submission template
# note that this is intentionally multiline
#######################################################################
set(PYCICLE_JOB_SCRIPT_TEMPLATE "#!/bin/bash
set(PYCICLE_PBS_TEMPLATE "#!/bin/bash
Copy link
Owner

Choose a reason for hiding this comment

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

same here - I renamed it to PYCICLE_JOB_SCRIPT_TEMPLATE to make it easier to search/replace and switch between slurm/pbs /other

config/dca/cadesGPUCondo.cmake Outdated
@@ -1,5 +1,4 @@
# Copyright (c) 2018 Peter Doak
# Copyright (c) 2017-2018 John Biddiscombe
# Copyright (c) 2018 John Biddiscombe, Peter Doak
Copy link
Owner

Choose a reason for hiding this comment

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

trivial detail, but I quite like the hpx convention where each contributor puts his name on a separate line and edits the dates accordingly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yest that's more informative, I'll switch it back

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So last person to update file on top?

dashboard_script.cmake
@@ -92,15 +94,17 @@ endif()
#####################################################################
# if this is a PR to be merged with master for testing
#####################################################################
if (NOT PYCICLE_PR STREQUAL "master")
if (NOT PYCICLE_PR STREQUAL "${PYCICLE_MASTER}")
Copy link
Owner

Choose a reason for hiding this comment

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

good catch

pycicle_params.py Outdated
'PYCICLE_CDASH_HTTP_PATH',
'PYCICLE_BUILD_STAMP',
'PYCICLE_COMPILER_SETUP',
'PYCICLE_SLURM',
Copy link
Owner

Choose a reason for hiding this comment

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

same comment here - see refactoroed slurm/pbs option

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes part of the point here is to support some config error catching.

pycicle_params.py Outdated
@@ -0,0 +1,79 @@
# Copyright (c) 2017-2018 John Biddiscombe, Peter Doak
Copy link
Owner

Choose a reason for hiding this comment

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

wow. I'd better test this right away. all the settings in one place. good idea

@PDoakORNL
Copy link
Collaborator Author

Ok fixed those issues.

@PDoakORNL
more current GPU config than master
7bec549
@PDoakORNL
first stage of modularizing config/args/params
5ca2999
@PDoakORNL
debug_print and what it does set in PycicleParams
1c749fe
@PDoakORNL
almost runs pyc_p somehow out of scope at main.
4537949
@PDoakORNL
a more idiomatic layout for purposes of sanity
dd8bbf9
@PDoakORNL
PBS without a job submission template=>fatal error
e11aa8b
@PDoakORNL
necessary changes for running on non-org repo
570a723 
also config to run on alternate cdash, although that still doesn't work
@PDoakORNL
Support for non-master branch i.e. feature trunk
25b42f3 
pycicle.py: output and support for connecting personal repos
README.md: Beginning of explanation of this feature and althernate cdash server feature
@PDoakORNL
appears necessary to pass compiler feature tests
8aa1083 
cmake 3.10 gcc-5.3 openmpi 3.0
@PDoakORNL
Updated to reflect review
4fda272
@PDoakORNL
removed redundant commented code
22e16be
@biddisco
Copy link
Owner 

jb-s76:~/pycicle$ python3 ~/src/pycicle/pycicle.py -m jb-laptop -P hpx -d
Traceback (most recent call last):
  File "/home/biddisco/src/pycicle/pycicle.py", line 26, in <module>
    from pycicle_params import PycicleParams
  File "/home/biddisco/src/pycicle/pycicle_params.py", line 74
    m = re.findall(setting + ur"\s*\"(.+?)\"", line)
                                            ^
SyntaxError: invalid syntax

must be something to do with python 2 vs python 3. I use python3 on my machines.

I'll try to fix it ...

@biddisco
Copy link
Owner

so it seems python 3 allows u"string" or r"string", but not ur"string", so I'll use u"string"

I've got a problem with default args. pycicle compiler_type is set to None and the default "gcc" is not substituted. Might as well make that a compulsory setting and remove the default value. Only needs me to add gcc to my jb-laptop config.

@biddisco biddisco merged commit 5cc62aa into biddisco:master May 25, 2018
@biddisco biddisco mentioned this pull request May 25, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@biddisco biddisco biddisco requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PDoakORNL @biddisco