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

Add check to run_mtl_loop that archived directories match the tiles-specstatus file #808

Merged
merged 6 commits into from Oct 30, 2023

Conversation

geordie666
Copy link
Contributor

@geordie666 geordie666 commented Oct 6, 2023

This PR adds a check_archiving() function that is used in run_mtl_loop to test whether archived tile directories match the ARCHIVEDATE column in the tiles-spectstatus.ecsv file. The check is restricted to bright and dark tiles from the Main Survey.

See #807 and desihub/desisurveyops#141 for more context.

run_mtl_loop now also includes a --noarchivecheck flag that can be used to bypass the archiving check (e.g., to speed up simulations or the alt-MTL process).

I tested this pretty extensively by:

  • Creating a mock up of the archive directories in /pscratch/sd/a/adamyers/arxivsandbox/tiles/archive/.
  • Copying the tiles-specstatus.ecsv file to /pscratch/sd/a/adamyers/arxivsandbox/ops/.
  • Moving the archive directories for tiles 20994 and 25442 to /pscratch/sd/a/adamyers/arxivsandbox/tiles/original/.
  • Removing tile 3333 from /pscratch/sd/a/adamyers/arxivsandbox/ops/tiles-specstatus.ecsv (the original file is retained at /pscratch/sd/a/adamyers/arxivsandbox/ops/tiles-specstatus.ecsv.orig).
  • Then running, e.g., run_mtl_loop DARK --zcatdir /pscratch/sd/a/adamyers/arxivsandbox/ --mtldir /pscratch/sd/a/adamyers/arxivsandbox/mtl

The resulting error message was:

ERROR:mtl.py:494:check_archiving: archived tiles not in specstatus file: [3333]; tiles in specstatus file that aren't archived: [20994 25442]

@araichoor: I'd appreciate a second pair of eyes on this PR before I tag desitarget and we make the check a standard part of daily ops.

I don't think this is critical to resolve on the timescale of the long weekend, though!

@coveralls
Copy link

coveralls commented Oct 6, 2023

Coverage Status

coverage: 56.075% (-0.2%) from 56.25% when pulling 9bf0351 on ADM-check-arxiv into 1e1034a on main.

@geordie666
Copy link
Contributor Author

Pinging @araichoor as a reminder. This still probably still isn't urgent yet, but it would be nice to merge this this week.

@araichoor
Copy link

sorry that fell off my radar.
I ll have a look, and comment what I can.

Copy link

@araichoor araichoor left a comment

Choose a reason for hiding this comment

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

ok, so here are minor comments/suggestions, which you can ignore if you think it s better to do so.

bin/run_mtl_loop Outdated Show resolved Hide resolved
doc/changes.rst Show resolved Hide resolved
py/desitarget/mtl.py Outdated Show resolved Hide resolved
py/desitarget/mtl.py Show resolved Hide resolved
py/desitarget/mtl.py Outdated Show resolved Hide resolved
py/desitarget/mtl.py Show resolved Hide resolved
py/desitarget/mtl.py Outdated Show resolved Hide resolved
@geordie666
Copy link
Contributor Author

Thanks for your careful review @araichoor. I addressed all of your comments. I went in the direction you suggested of splitting out the archiving check by main/bright and main/dark.

I've left one request for an opinion from @schlafly as part of my response to the final comment of your review. Otherwise I think this PR should be ready for merging.

I rechecked the code using the same test as detailed at the top of this thread.

@araichoor
Copy link

thanks for implementing my suggestions!

I just thought of a possible minor drawback of making the check on {survey,obscon} only, but I think there s nothing to change in the code.

I thought about the following:

  • the dailyops person runs run_mtl_loop for the {main,bright}, and the archiving check fails;
  • then one may want to verify the archiving situation for {main,dark} also, without running mtl_loop_run for {main,dark} (because if only {main,bright} tiles have archiving issue, {main,dark} would run smoothly, and for the time to resolve the issue, bright/dark would be "out-of-sync" from an mtl-point-of-view; but maybe that s fine because we d then sort out + fix the main,bright case right away);
  • but if we want to check the archiving for {main,dark} without running run_mtl_loop, one can just run from python:
from desitarget.mtl import check_archiving
check_archiving("DARK")

(and we could put that instruction in the dailyops wiki).

@geordie666
Copy link
Contributor Author

I think "dark" and "bright" being "out-of-sync" is fine, though, right?

As far as I know we treat "bright" and "dark" as completely separate surveys? So, although we've never just run run_mtl_loop DARK or run_mtl_loop BRIGHT (because of the way the instructions are structured on the wiki) there's nothing, in principal, that's actually wrong with just running one or the other?

Sometimes we run an MTL update where there are, e.g., some new dark tiles but no new bright tiles and the run_mtl_loop BRIGHT command just does nothing in that case.

Another way of looking at this (which is why I liked your suggestion) is that we might want to proceed with an MTL update for dark tiles even if there's an archiving issue for bright tiles.

Am I missing something that intertwines the bright and dark parts of the survey?

@araichoor
Copy link

'I think "dark" and "bright" being "out-of-sync" is fine, though, right?' => yes, sure, no big deal actually

'Am I missing something that intertwines the bright and dark parts of the survey?' => no you re right!

I was just slightly worried that we end up in a case where run_mtl_loop has been run for one program but not for another one; but you re totally right that those are fully disjoint, and that this actually already happens if we do an mtl update but one program has no new tiles.

sorry for the noise!

@geordie666 geordie666 merged commit ab1267e into main Oct 30, 2023
12 checks passed
@geordie666 geordie666 deleted the ADM-check-arxiv branch October 30, 2023 19:59
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.

None yet

4 participants