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

Canfar run v1.4 #677

Draft
wants to merge 168 commits into
base: develop
Choose a base branch
from
Draft

Conversation

martinkilbinger
Copy link
Contributor

@martinkilbinger martinkilbinger commented Mar 6, 2024

Summary

Changes to get shapepipe run on the canfar science portal. Used to create the SP v1.4 catalogue.

The PRs
martinkilbinger#3 ... martinkilbinger#14
were merged before to https://github.com/martinkilbinger/shapepipe-1.

Reviewer Checklist

Reviewers should tick the following boxes before approving and merging the PR.

  • The PR targets the develop branch
  • The PR is assigned to the developer
  • The PR has appropriate labels
  • The PR is included in appropriate projects and/or milestones
  • The PR includes a clear description of the proposed changes
  • If the PR addresses an open issue the description includes "closes #"
  • The code and documentation style match the current standards
  • Documentation has been added/updated consistently with the code
  • All CI tests are passing
  • API docs have been built and checked at least once (if relevant)
  • All changed files have been checked and comments provided to the developer
  • All of the reviewer's comments have been satisfactorily addressed by the developer

sfarrens and others added 30 commits November 16, 2022 09:36
Conflicts:
	Dockerfile
	environment.yml
	scripts/sh/init_canfar.sh

1. Retrieve `ShapePipe` result files
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Obsolete VM system on canfar.

@@ -0,0 +1,51 @@
## Retrieve files from VOspace
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isolated vos HOWTO into separate doc file.

@@ -4,30 +4,30 @@ channels:
dependencies:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to library inconsistencies I removed most of the set versions. We can try to find an updated set that works on our different platforms.

@@ -25,20 +25,6 @@ libpng_ver="1.6.37"
mpi4py_ver="3.1.3"
openblas_ver="0.3.18"

# SExtractor Package
Copy link
Contributor Author

Choose a reason for hiding this comment

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

astromatic packages now installed via conda.

@@ -193,7 +117,7 @@ check_conda() {
CONDA_VERSION_MAJOR=$(cut -d'.' -f1 <<<$CONDA_VERSION)
CONDA_VERSION_MINOR=$(cut -d'.' -f2 <<<$CONDA_VERSION)
CONDA_VERSION_PATCH=$(cut -d'.' -f3 <<<$CONDA_VERSION)
CONDA_SH=/etc/profile.d/conda.sh
CONDA_SH=/opt/conda/etc/profile.d/conda.sh
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The correct path on canfar; can we do this in a general way?

@@ -0,0 +1,62 @@
#
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From Sebastien Fabbro.

@@ -66,7 +66,16 @@ def psfex_interp_runner(
module_config_sec,
'ME_DOT_PSF_DIR',
)
dot_psf_dir = get_last_dir(run_dirs['run_log'], module)
module_name = module.split(":")[-1]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added flexibility to use "last:dir" and "all:dir" as search strings.

self._data = cat_file.get_data()
try:
self._data = cat_file.get_data()
except:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

catch error, e.g. if file is corrupt or has zero size.

image_dir + '/' + image_pattern + '-'
+ exp_name + '-' + str(ccd) + '.fits'
)
# Look for input image
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug fix: Need to go through list of input directories instead of just one.

last_dir = get_last_dir(run_dirs['run_log'], module)
image_dir.append(last_dir)
module_name = module.split(":")[-1]
if "last" in module:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added flexibility to input run(s)

@@ -128,5 +128,10 @@ def create_arg_parser():
help='configuration file name',
)

optional.add_argument(
'-e',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new option, to run shapepipe on exclusively one single image ID.

id_to_test = f"-{self._exclusive.replace('.', '-')}"
if number == id_to_test:
if self._verbose:
print(f"-- Using exclusive number {self._exclusive} ({id_to_test})")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test print, should probably be removed.

@@ -1120,6 +1135,9 @@ def _format_process_list(
])
process_list.append(process_items)

if len(process_list) == 0:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Catching an error that sometimes occurs, e.g. when something is wrong with input files or directories.
If not, caused unexpected behaviour later on. Check and error message could still be improved.

@martinkilbinger martinkilbinger marked this pull request as draft March 6, 2024 18:03
Copy link
Member

@sfarrens sfarrens left a comment

Choose a reason for hiding this comment

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

Hey @martinkilbinger I have some questions about the Dockerfile (see opened threads). In order to move things along a bit faster, it would probably be easier to implement these changes in #600, but I would like to understand everything first.

Comment on lines +45 to +46
COPY ./environment.yml ./
COPY install_shapepipe README.rst setup.py setup.cfg ./
Copy link
Member

Choose a reason for hiding this comment

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

@martinkilbinger is there any particular reason you are copying things to / rather than e.g. /home?

Comment on lines +52 to +53
COPY shapepipe ./shapepipe
COPY scripts ./scripts
Copy link
Member

Choose a reason for hiding this comment

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

Again, is there any reason you only copy these directories and now the whole repo?

COPY shapepipe ./shapepipe
COPY scripts ./scripts

RUN source activate shapepipe
Copy link
Member

Choose a reason for hiding this comment

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

The following line will generate an Image that will launch containers with ShapePipe pre-activated.

RUN echo "conda activate shapepipe" >> ~/.bashrc


COPY ./environment.yml ./
COPY install_shapepipe README.rst setup.py setup.cfg ./
RUN touch ./README.md
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

COPY install_shapepipe README.rst setup.py setup.cfg ./
RUN touch ./README.md

RUN conda update -n base -c defaults conda -c defaults
Copy link
Member

Choose a reason for hiding this comment

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

Was this needed to get things working?

ARG CC=gcc-9
ARG CXX=g++-9

# gcc < 10 is required to compile ww
Copy link
Member

Choose a reason for hiding this comment

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

I guess you checked this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix documentation Improvements or additions to documentation enhancement New feature or request
Projects
Status: To do
Development

Successfully merging this pull request may close these issues.

2 participants