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

stackSentinel background task fix #271

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

hardreddata
Copy link
Contributor

There is a problem with #223 in which any tasks running in the background will continue in the background when the next step of the process begins (eg: run_03 can bleed into run_04). This causes issues when some steps try and use incomplete data.

The proposed solution writes a final wait in any shell scripts which leverage num_process.

I welcome any feedback.

Copy link
Member

@yunjunz yunjunz left a comment

Choose a reason for hiding this comment

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

Unrelated to this specific PR, but I would suggest to simplify write_wrapper_config2run_file() by:

  • set default line_cnt=1
  • change if line_cnt == numProcess to if line_cnt % numProcess == 0

In this way, the input line_cnt is the actual line index, so we don't need to return this variable, thus, shorten all calls of the func from:

line_cnt = configObj.write_wrapper_config2run_file(configName, line_cnt)

to

configObj.write_wrapper_config2run_file(configName, line_cnt)

@hardreddata
Copy link
Contributor Author

@yunjunz I would be happy to change

        line_cnt = 1
        configObj.write_wrapper_config2run_file(configName, line_cnt)

to

        configObj.write_wrapper_config2run_file(configName)

in the three occurrences. I left as above as it might be simpler to follow.

Presently the stackSentinel.py num_process is buggy so I think good to get this merged. Apologies for the delay.

contrib/stack/topsStack/Stack.py Outdated Show resolved Hide resolved
contrib/stack/topsStack/Stack.py Outdated Show resolved Hide resolved
@yunjunz
Copy link
Member

yunjunz commented Jul 26, 2022

Thank you for getting back to this @RussellGrew. Could you do the following?

  1. Rebase this PR against the latest main branch, as the stack processor has been updated and GitHub flagged conflicts.
  2. Apply the same enumerate style call to the new functions added within the class run(object).

@yunjunz I would be happy to change

        line_cnt = 1
        configObj.write_wrapper_config2run_file(configName, line_cnt)

to

        configObj.write_wrapper_config2run_file(configName)

in the three occurrences. I left as above as it might be simpler to follow.

I would prefer the latter one. We don't need to show the details if we don't have to.

@hardreddata
Copy link
Contributor Author

@yunjunz I think that is everything.

@yunjunz
Copy link
Member

yunjunz commented Jul 27, 2022

These two still stand I believe:

  1. Rebase this PR against the latest main branch, as the stack processor has been updated and GitHub flagged conflicts.
  2. Apply the same enumerate style call to the new functions added within the class run(object). E.g., filtIon(). They will be visible in this PR after rebasing.

@hardreddata
Copy link
Contributor Author

hardreddata commented Jul 27, 2022

@yunjunz see 4ffbfff and 8971f52

Though this was the first time I have attempted to update a branch for an in progress PR. Hopefully it went ok.

Edit - I see that it did not. Leave it with me.

@hardreddata
Copy link
Contributor Author

That should be much closer.

You did this to yourself with your notes of encouragement on https://mintpy.readthedocs.io/en/latest/#5_contributing

I see the CI has now passed.

I am not familiar with these new ionosphere steps. I checked that the config is written out, and that step 17 and 18 which consider the numProcess are written correctly.

Cheers.

Copy link
Member

@yunjunz yunjunz left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @RussellGrew!

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.

2 participants