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

Issue 189 #208

Merged
merged 6 commits into from
Feb 2, 2024
Merged

Issue 189 #208

merged 6 commits into from
Feb 2, 2024

Conversation

o-smirnov
Copy link
Member

Please review and merge #207 first, as this is based off that branch.

Adds bindings of non-current paths into singularity, fixing #189 which tripped up @Athanaseus a few times.

Also adds access_parent_dir and write_parent_dir fields to Parameter. I quickly realized this was needed for "funny" stuff like CASA flagman (because if it's accessing foo/bar.MS, it also needs access to foo/ itself, so it can create the flagversions table alongside the MS).

@o-smirnov
Copy link
Member Author

Do not merge yet -- see a potential bug.

@o-smirnov
Copy link
Member Author

OK seems fine now. But let my recipe run on ilifu for a few more steps to make sure the various directory bindings work as intended.

@SpheMakh
Copy link
Collaborator

Could you also add this test; the file is empty? tests/stimela_tests/test_slurm_config.yml

@o-smirnov
Copy link
Member Author

Could you also add this test; the file is empty? tests/stimela_tests/test_slurm_config.yml

It's empty by design. The user can replace it with a non-empty config file to specify non-default slurm options. We don't have an _incude_if_exists statement, so it's easier to have an empty file.

@o-smirnov
Copy link
Member Author

A few more small fixes incoming... let me run an E2E workflow on ilifu before we merge (and maybe wait for @Athanaseus to finish his test on the pleiades cluster).

@o-smirnov
Copy link
Member Author

OK, I'm happy with how it works. All filesystems appear to be mounting correctly. @Athanaseus or @SpheMakh please review and merge. I'll start other fixes on a branch off this one.

Athanaseus
Athanaseus previously approved these changes Feb 1, 2024
@o-smirnov o-smirnov dismissed Athanaseus’s stale review February 1, 2024 21:43

The merge-base changed after approval.

@Athanaseus Athanaseus self-requested a review February 1, 2024 21:56
@o-smirnov o-smirnov dismissed Athanaseus’s stale review February 1, 2024 21:57

The merge-base changed after approval.

Athanaseus
Athanaseus previously approved these changes Feb 1, 2024
@o-smirnov
Copy link
Member Author

@Athanaseus looks like you reviewed, then cancelled your review -- what's up?

Athanaseus
Athanaseus previously approved these changes Feb 2, 2024
@o-smirnov o-smirnov dismissed Athanaseus’s stale review February 2, 2024 06:58

The merge-base changed after approval.

@Athanaseus
Copy link
Contributor

When I approve, it does exactly that. Not sure why.
@Athanaseus dismissed review on this pull request.

Athanaseus
Athanaseus previously approved these changes Feb 2, 2024
@Athanaseus
Copy link
Contributor

ok i see now, a file changed, and GitHub won't let me approve without reviewing it.

@o-smirnov o-smirnov dismissed Athanaseus’s stale review February 2, 2024 07:07

The merge-base changed after approval.

@o-smirnov
Copy link
Member Author

That's odd, my last commit was yesterday, so why is it dismissing your review today? Could you please try approving again?

Athanaseus
Athanaseus previously approved these changes Feb 2, 2024
Copy link
Contributor

@Athanaseus Athanaseus left a comment

Choose a reason for hiding this comment

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

LGTM!

@o-smirnov o-smirnov dismissed Athanaseus’s stale review February 2, 2024 09:06

The merge-base changed after approval.

@o-smirnov
Copy link
Member Author

OK I think the review cancellation is a github bug. I'll force-merge.

@o-smirnov o-smirnov merged commit 9045b6d into master Feb 2, 2024
4 checks passed
@o-smirnov o-smirnov deleted the issue-189 branch February 2, 2024 10:18
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.

3 participants