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

Better sync of pixel tasks #590

Merged
merged 3 commits into from Apr 19, 2018
Merged

Better sync of pixel tasks #590

merged 3 commits into from Apr 19, 2018

Conversation

tskisner
Copy link
Member

The DataBase.sync() method scans the filesystem and rebuilds the database state of all tasks. This works fine for all tasks through cframes. For spectra and redshift tasks, the dependencies on cframes are tracked in the healpix_frame table. This PR attempts to reconstruct the state of these tasks (i.e. waiting, ready, done) based on the filesystem and on the current status of the healpix_frame table. The logic here is the desire to do:

$>  find spectra-64 -name 'zbest*' -exec rm '{}' \;
$>  desi_pipe sync

And have the state of the spectra and redshift tasks be correct. The sync command now also calls the getready() method.

Additionally there was a bug in the order of tasktypes returned by db.task_types() (used in several places). The tasktype order was not the same as the execution order. This should close #558 and close #563 .

…d redshift tasks. Call getready() at the end of the sync method to propagate dependencies. When getting the list of all tasks, use the default chain order rather than dictionary keys. This ensures that the tasks are processed in execution order.
@tskisner
Copy link
Member Author

Offline feedback from @sbailey: for this short term fix, if the spectra and redshift files exist at all, then we should assume they are done.

This will give us the wrong answer if (for example) we have completed cframes that have not yet been added to spectra and redshifts, and then we run desi_pipe sync. However, it would handle the case of files being created outside the pipeline that we then want to include in the DB.

In the case where files were removed, we would still move the state back to a previous one cleanly.

The long term solution is obviously to open the fibermaps in each file and see if the cframe contents are complete.

@tskisner
Copy link
Member Author

@sbailey, this should now implement your desired behavior- can you test again on your mini sim? I verified that using one day of a month production that I can add / remove spectra and zbest files and sync sets the expected state. One note- if spectra files are missing but zbest files exist, then it resets the state back to "spectra = ready, redshift = waiting". Presumably if you are creating zbest files outside of the pipeline then you also have spectra files, so this should not be a problem.

@sbailey
Copy link
Contributor

sbailey commented Apr 18, 2018

I confirm that it now works for my minitest case. Thanks.

Please add some comments in the code about what the 1/2/3/4 magic number states mean. Some of those are mentioned # We are all done (state 3) but others are more implicit "# We are only at state 2". I think the mapping is:

  • 0 = some cframe inputs are missing (but not necessarily all)
  • 1 = all required cframes have been generated
  • 2 = spectra file has been updated
  • 3 = zbest file has been updated

When new data arrive, do all covered healpix reset back to state 0 because some cframe files are missing, even if others exist and have been processed all the way through redshifts? Answer that with code comments, not just by answering me here. If those states are already clearly documented in comments elsewhere, it's ok if this updating code just references "see x.y.z for explanation of the states".

Copy link
Contributor

@sbailey sbailey left a comment

Choose a reason for hiding this comment

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

works; ok to merge, but please add some additional comments explaining 0/1/2/3 states either as part of this PR or as part of a future update.

@tskisner
Copy link
Member Author

The issue of comments and documentation is certainly valid, and documenting the states in the healpix_frame table and when those are changed should be done. It is a much bigger issue than just the DataBase.sync() method changed here (see #531).

For the record here: completion of new cframes changes pixel state 0 --> 1. Completion of spectra changes 1 --> 2. Completion of redshifts changes 2 --> 3.

So no spectra files will be moved from "done" back to "ready" until after a new cframe file that touches that pixel is completed.

@tskisner
Copy link
Member Author

I added more details to #531. Merging this now.

@tskisner tskisner merged commit 4919f9e into master Apr 19, 2018
@tskisner tskisner deleted the pixsync branch April 19, 2018 13:17
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.

desi_pipe sync doesn't sync redrock output desi_pipe sync waiting/ready inconsistencies
2 participants