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

Julia build in container #45

Merged
merged 72 commits into from
Aug 23, 2023
Merged

Julia build in container #45

merged 72 commits into from
Aug 23, 2023

Conversation

tdivoll
Copy link
Collaborator

@tdivoll tdivoll commented Jul 21, 2023

This PR works out the precompiling and building of IFTPipeline in the container.

@tdivoll tdivoll added the enhancement New feature or request label Jul 21, 2023
@tdivoll tdivoll self-assigned this Jul 21, 2023
@tdivoll tdivoll marked this pull request as ready for review July 21, 2023 00:30
@tdivoll tdivoll changed the base branch from feat-docker-julia to main July 21, 2023 00:31
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (4ef5326) 77.92% compared to head (2596540) 77.92%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #45   +/-   ##
=======================================
  Coverage   77.92%   77.92%           
=======================================
  Files           8        8           
  Lines         299      299           
=======================================
  Hits          233      233           
  Misses         66       66           
Files Changed Coverage Δ
src/h5.jl 97.18% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tdivoll tdivoll mentioned this pull request Aug 17, 2023
Project.toml Outdated
LoggingExtras = "1.0.1"
PyCall = "1.96.1"
Pkg = "1.9.0"
TOML = "1.0.3"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TOML = "1.0.3"
TOML = "1.0.3"

README.md Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Why delete this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, I think it just got inadvertently deleted when I was testing (wiping the downloaded resources).

Comment on lines 82 to 86
# logger = setuplogger(logoption, command)

with_logger(logger) do
@time command_func(; command_args...)
end
# with_logger(logger) do
@time command_func(; command_args...)
# end
Copy link
Member

Choose a reason for hiding this comment

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

Why comment out the logger? If it's not needed, some deps (LoggingExtras) could also be dropped.

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 think it was the issue with the logger trying to write in the read-only container. I can retest it and see if it will work now that we've updated some other code. It's possible that we have the right bindings now and it might work.

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 was able to add a binding so that it will write the logs to workflow/report!

[[pulljuliaimage]]
script = """
apptainer build --force $project_dir/icefloetracker-julia.simg docker://brownccv/icefloetracker-julia:pr-45
"""
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After merging, I'll have to start a new PR and change the image tag to main rather than pr-45.

Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

👍
Added a few things to consider.

RUN chmod a+x /opt/ice-floe-tracker-pipeline/workflow/scripts/ice-floe-tracker.jl
COPY workflow/scripts/ice-floe-tracker.jl /usr/local/bin/ice-floe-tracker.jl

RUN chmod a+x /usr/local/bin/ice-floe-tracker.jl
Copy link
Member

Choose a reason for hiding this comment

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

Is this really required? I see that the command used to run the ice-floe-tracker.jl script includes 'julia' which is probably required to pass the -t auto flag to enable multithreading.

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 think it has to do with the permissions in a singularity container. Things work for container permissions when we copy to/usr/local/bin. I was running into issues with running it directly from the repo.

Comment on lines +17 to +26
[compat]
ArgParse = "1.1.4"
Folds = "0.2.8"
HDF5 = "0.16.15"
IceFloeTracker = "0.2.1"
LoggingExtras = "1.0.1"
PyCall = "1.96.1"
Pkg = "1.9.0"
TOML = "1.0.3"

Copy link
Member

Choose a reason for hiding this comment

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

👍

Comment on lines +32 to 38
5. Export SOIT username/password to environment variable
- [ ] From your home directory `nano .bash_profile`
- [ ] add `export HISTCONTROL=ignoreboth` to the bottom of your .bash_profile
* this will ensure that your username/password are not stored in history
* when exporting the following environment variables, there __must__ be a space in front of each command
- [ ] ` export SPACEUSER=<firstname>_<lastname>@brown.edu`
- [ ] ` export SPACEPSWD=<password>`
Copy link
Member

Choose a reason for hiding this comment

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

From Josh's talk yesterday, I was thinking maybe secrets could be used for this. Not sure if it's possible.

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'll make an issue for looking into this. That might be a good way to do it. Talking with @broarr, it seemed like this use case was best to not store anything in a file.

Comment on lines +63 to 78
singularity exec --bind $landmask_dir:/tmp,$report_dir:/usr/local/bin/../report $project_dir/icefloetracker-julia.simg $julia_exec -t auto /usr/local/bin/ice-floe-tracker.jl landmask $fetchdata_dir /tmp
"""
[[preprocess]]
script = """
julia -t auto $project_dir/workflow/scripts/ice-floe-tracker.jl preprocess -t $fetchdata_dir/truecolor -r $fetchdata_dir/reflectance -l $landmask_dir -p $soit_dir -o $preprocess_dir
singularity exec --bind $preprocess_dir:/tmp,$report_dir:/usr/local/bin/../report $project_dir/icefloetracker-julia.simg $julia_exec -t auto /usr/local/bin/ice-floe-tracker.jl preprocess -t $fetchdata_dir/truecolor -r $fetchdata_dir/reflectance -l $landmask_dir -p $soit_dir -o /tmp
"""
[[extractfeatures]]
script = """
julia -t auto $project_dir/workflow/scripts/ice-floe-tracker.jl extractfeatures -i $preprocess_dir -o $preprocess_dir --minarea $minfloearea --maxarea $maxfloearea
singularity exec --bind $preprocess_dir:/tmp,$report_dir:/usr/local/bin/../report $project_dir/icefloetracker-julia.simg $julia_exec -t auto /usr/local/bin/ice-floe-tracker.jl extractfeatures -i $preprocess_dir -o /tmp --minarea $minfloearea --maxarea $maxfloearea
"""
[[tracking]]
script = """
julia -t auto $project_dir/workflow/scripts/ice-floe-tracker.jl track --imgs $preprocess_dir --props $preprocess_dir --deltat $preprocess_dir --output $tracker_dir
singularity exec --bind $tracker_dir:/tmp,$report_dir:/usr/local/bin/../report $project_dir/icefloetracker-julia.simg $julia_exec -t auto /usr/local/bin/ice-floe-tracker.jl track --imgs $preprocess_dir --props $preprocess_dir --deltat $preprocess_dir --output /tmp
"""
[[exportH5]]
script = """
Copy link
Member

Choose a reason for hiding this comment

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

Great work figuring out the bindings!

config/cylc_hpc/flow.cylc Outdated Show resolved Hide resolved
Add line ending

Co-authored-by: Carlos Paniagua <[email protected]>
@tdivoll tdivoll merged commit 806d516 into main Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants