General improvements to MTL functionality.

[geordie666]

This PR incorporates a couple of useful features to help MTL functionality. It:

• Removes any checks of the data model in mtl.inflate_ledger(). This translates to a significant speed-up, which approaches 50x in many use cases.
• Adds a unit test to check the desitarget ZWARN bit-mask agrees with the redrock version.
  • This should prevent desitarget from neglecting any changes made to ZWARN in redrock.

[geordie666]

@weaverba137: I added a unit test that requires redrock as a dependency, and tests are failing. So, I must need to explicitly add redrock as part of the github workflows.

As I'm still getting used to github actions, can you give me some advice on how best to achieve this? Is it as simple as adding:

python -m pip install git+https://github.com/desihub/redrock.git@${REDROCK_VERSION}#egg=redrock

with an appropriate REDROCK_VERSION here and here?

Also, is there a way to make REDROCK_VERSION always correspond to the latest tagged version? That would be most useful for the specific test I've added.

[weaverba137]

@geordie666 , if you use desispec as a template, you should be able to find examples of how to install redrock.

[weaverba137]

PS, you should pick a specific REDROCK_VERSION. The only way I know to obtain that information programmatically would be a call to the GitHub API, and I'm not going to try to figure out how to do that. I think that is out of scope.

[sbailey]

I haven't implemented github actions myself so I have no additional technical insights to offer, but I think it would be fine for you to test against redrock master (soon to be "main") in lieu of the latest tag.

[geordie666]

Thanks, both. @sbailey: It looks to me like desispec doesn't actually import redrock anywhere for testing. Do you know if we have any unit tests that currently import redrock? The desispec workflows file is essentially identical to the desitarget file.

[weaverba137]

What functionality is specifically needed from redrock, and would that functionality be better off moved to a separate package?

[geordie666]

@weaverba137: Your comment made me realize that I'd backed myself into a corner, here (thanks).

I wanted to add a unit test that checked that a bit-mask in desitarget didn't get out of sync with a bit-mask in redrock. The reason I set up a separate version of the bit-mask in desitarget in the first place was because I didn't want to make redrock a dependency of desitarget.

The irony in this PR is that I'm discussing making redrock a dependency of desitarget for unit tests, exactly what I was trying to avoid!

I see two ways forward:

1. I update the unit test to check if redrock is installed before proceeding. That way, I can keep track of the bit-masks when I'm running in my own environment at NERSC (and presumably @sbailey will also then automatically be able to track the bit-mask correspondence in his end-to-end integration tests).
2. We move the particular bit-mask (ZWARN) from redrock into desiutil. I think @sbailey was very much against this as he wants to keep redrock separate from the desi modules.

I think 1. will probably meet all of the desiderata without being too much extra work for anyone.

[weaverba137]

@sbailey, does redrock not depend on desiutil at all, like specter? If redrock does depend on desiutil, then the objection to moving ZWARN to desiutil doesn't make sense to me.

@geordie666, glad to get you out of that corner.

[geordie666]

@weaverba137: Should I proceed with my plan to add a check that redrock is installed in the new unit test (for this PR)? If we did choose to refactor the ZWARN bit-mask to be in desiutil, that entire unit test would be rendered moot anyway, and it's a self-contained test, so we could easily just remove it later.

[geordie666]

(oh, and I may just be erroneously putting words in @sbailey's mouth, so we should definitely see if he genuinely has any objections to option 2.)

[sbailey]

@weaverba137 redrock is one of our experiment agnostic packages and thus only optionally depends upon desiutil, e.g. so that eboss could use redrock without needing desiutil. The zwarning bits that @geordie666 wants to check for consistency are part of the non-DESI-specific parts of redrock, and thus are not eligible for moving to desiutil.

@geordie666 I originally intended that this test would be optional, so that someone (including github) could run the rest of the test suite without needing redrock installed. The integration tests at NERSC have redrock installed and thus would run the test once a day, which I think is sufficient for rarely changed bit definitions. desispec has similar "optional tests" for specter and sqlalchemy using a pattern like your option (1):

try:
    import redrock
    redrock_available = True
except ImportError:
    redrock_available = False

...

    @unitest.skipUnless(redrock_available, "redrock not installed, skipping bit consistency test")
    def test_redrock_bits(...):
        ....

Even if you do include redrock in the github tests, I think it would be useful to include that test check so that other desitarget users/developers can run the desitarget tests locally without requiring redrock.

[geordie666]

@sbailey: Thanks, that all makes sense and I can see the way to proceed now.

[weaverba137]

This all sounds fine to me. When unit tests are run as part of the nightly integration test, let's check that these tests are in fact running.

[geordie666]

OK, that seemed to work. I'll merge this in about an hour. If the new check:

test_zwarn_in_sync (desitarget.test.test_mtl.TestMTL)
Check redrock and desitarget ZWARN bit-masks are consistent. ... ok

is skipped by the nightly integration test then let me know and I can try to determine what happened.

[weaverba137]

desitarget tests in the nightly integration test are failing with this error message:

======================================================================
FAIL: test_zwarn_in_sync (desitarget.test.test_mtl.TestMTL)
Check redrock and desitarget ZWARN bit-masks are consistent.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/global/common/software/desi/cori/desiconda/20200801-1.4.0-spec/code/desitarget/master/py/desitarget/test/test_mtl.py", line 201, in test_zwarn_in_sync
    self.assertTrue(bitname in dtbitnames,
AssertionError: False is not true : missing ZWARN bit RESERVED16 from redrock

----------------------------------------------------------------------

[geordie666]

Thanks, @weaverba137. This failure is because I was locally testing against the stable version of redrock in desiconda instead of against master. The good news is, this is exactly what the test was designed to catch, so it works well.

I'll fix this in a new branch in about an hour. It's a quick fix.