Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-1439

afw tests use the same name for a dummy file: test.fits

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw

      Description

      The afw package uses a file named: test.fits in multiple testers. If the user sets up the build to use multiple CPU (-j #), then there is the risk that the shared filename will be affected by more than one tester at a time.

      In the case which provoked this Issue, the tester: testSimpleTable.py, reported that the file was missing. A simple rerun managed to get past the error.

      I recommend that the different testers use uniquely named demo files.

        Attachments

          Issue Links

            Activity

            Hide
            rhl Robert Lupton added a comment -

            git blame reveals that this is your doing. Could you fix it?

            Show
            rhl Robert Lupton added a comment - git blame reveals that this is your doing. Could you fix it?
            Hide
            rhl Robert Lupton added a comment -

            I see that there's a similar problem in testWcsFitsTable.py that should be fixed at the same time. I didn't do an exhaustive audit, but there may be other cases too.

            I also noticed that test.fits wasn't cleaned up if the tests failed. Is this deliberate? If so, there should probably be a comment or a note printed to stdout that tells the user that the file has been left behind for debugging.

            Show
            rhl Robert Lupton added a comment - I see that there's a similar problem in testWcsFitsTable.py that should be fixed at the same time. I didn't do an exhaustive audit, but there may be other cases too. I also noticed that test.fits wasn't cleaned up if the tests failed. Is this deliberate? If so, there should probably be a comment or a note printed to stdout that tells the user that the file has been left behind for debugging.
            Hide
            pgee Perry Gee added a comment -

            This should be a simple review. Just some name changing to insure uniqueness. Not sure if you are setup yet to do a review, but I assume so, since you are on the list of reviewers.

            The test framework (I am told) only runs an individual .py file in the test directory only once at any given time (never concurrently), so the name uniqueness just has to be good enough to not conflict with any other test. Since the files are generally stored beneath the package/tests directory (in this case afw/tests), only other tests in that directory have a potential to conflict.

            Show
            pgee Perry Gee added a comment - This should be a simple review. Just some name changing to insure uniqueness. Not sure if you are setup yet to do a review, but I assume so, since you are on the list of reviewers. The test framework (I am told) only runs an individual .py file in the test directory only once at any given time (never concurrently), so the name uniqueness just has to be good enough to not conflict with any other test. Since the files are generally stored beneath the package/tests directory (in this case afw/tests), only other tests in that directory have a potential to conflict.
            Hide
            jbosch Jim Bosch added a comment -

            Changes look fine to me; only comment would be that you should probably squash the two commits together using git rebase -i before merging, as there's no need for the second commit to be preserved in the history as separate from the first.

            John Swinbank, the usual procedure for responding to a review would be to comment here, then transition the ticket to "Review Complete" when you're satisfied (either when the code is in all fixed, or when your comments were trivial enough that you don't feel you need to see it again after it's fixed). I'll let you do that here.

            Show
            jbosch Jim Bosch added a comment - Changes look fine to me; only comment would be that you should probably squash the two commits together using git rebase -i before merging, as there's no need for the second commit to be preserved in the history as separate from the first. John Swinbank , the usual procedure for responding to a review would be to comment here, then transition the ticket to "Review Complete" when you're satisfied (either when the code is in all fixed, or when your comments were trivial enough that you don't feel you need to see it again after it's fixed). I'll let you do that here.
            Hide
            swinbank John Swinbank added a comment - - edited

            I agree with Jim that these commits should be squashed before merging.

            What should the failure mode be if testDM384.fits already exists? Will catalog.writeFits() clobber it, or will the test fail? If the former, is that desired behaviour?

            If the test fails for another reason, do you want to clean up testDM384.fits or leave it lying around for forensics?

            I'm not sure if this is within the scope of this review, but: structurally, this looks more like three tests than one to me. That's ugly, and there's a lot of repeated code. You could consider restructuring things to avoid the repetition and make the cleanup automatic. Something like:

            class TestDM384(unittest.TestCase):
                def setUp(self):
                    self.filename = "testDM384.fits"
                    self.schema = lsst.afw.table.Schema()
                    self.schema.setVersion(5)
                    self.schema.addField("f1", doc="f1a", type="I")
                    self.schema.addField("f2", doc="f2a", type="Flag")
                    self.schema.addField("f3", doc="f3a", type="ArrayF", size=4)
             
                def tearDown(self):
                    os.unlink(self.filename)
             
                def _run_test(self, catalog):
                    self.assertEqual(catalog.getTable().getVersion(), 5)
                    catalog.writeFits(self.filename)
                    # now read the table just written to disk, and see if it reads back correctly
                    catalog = catalog.readFits(self.filename)
                    metadata = catalog.getTable().getMetadata()
                    self.assertEqual(catalog.schema.getVersion(), 5)
                    self.assertFalse(metadata == None)
                    self.assertFalse(metadata.exists("AFW_TABLE_VERSION"))
             
                def test_BaseCatalog(self):
                    self._run_test(BaseCatalog(self.schema))
             
                def test_SimpleCatalog(self):
                    self._run_test(SimpleCatalog(self.schema))
             
                def test_SourceCatalog(self):
                    self._run_test(SourceCatalog(self.schema))

            (I've not actually tested the above, so it's probably riddled with typos etc, and I'm not sure if there's a substantive difference between lsst.afw.table.SimpleTable.makeMinimalSchema and lsst.afw.table.Schema, but you get the gist.)

            This goes significantly beyond the immediate scope of this issue, though, so I don't think it has to be done before this is merged. (Indeed, I'm not even sure if I'm allowed to request it here, or if I should open a new issue...)

            Show
            swinbank John Swinbank added a comment - - edited I agree with Jim that these commits should be squashed before merging. What should the failure mode be if testDM384.fits already exists? Will catalog.writeFits() clobber it, or will the test fail? If the former, is that desired behaviour? If the test fails for another reason, do you want to clean up testDM384.fits or leave it lying around for forensics? I'm not sure if this is within the scope of this review, but: structurally, this looks more like three tests than one to me. That's ugly, and there's a lot of repeated code. You could consider restructuring things to avoid the repetition and make the cleanup automatic. Something like: class TestDM384(unittest.TestCase): def setUp( self ): self .filename = "testDM384.fits" self .schema = lsst.afw.table.Schema() self .schema.setVersion( 5 ) self .schema.addField( "f1" , doc = "f1a" , type = "I" ) self .schema.addField( "f2" , doc = "f2a" , type = "Flag" ) self .schema.addField( "f3" , doc = "f3a" , type = "ArrayF" , size = 4 )   def tearDown( self ): os.unlink( self .filename)   def _run_test( self , catalog): self .assertEqual(catalog.getTable().getVersion(), 5 ) catalog.writeFits( self .filename) # now read the table just written to disk, and see if it reads back correctly catalog = catalog.readFits( self .filename) metadata = catalog.getTable().getMetadata() self .assertEqual(catalog.schema.getVersion(), 5 ) self .assertFalse(metadata = = None ) self .assertFalse(metadata.exists( "AFW_TABLE_VERSION" ))   def test_BaseCatalog( self ): self ._run_test(BaseCatalog( self .schema))   def test_SimpleCatalog( self ): self ._run_test(SimpleCatalog( self .schema))   def test_SourceCatalog( self ): self ._run_test(SourceCatalog( self .schema)) (I've not actually tested the above, so it's probably riddled with typos etc, and I'm not sure if there's a substantive difference between lsst.afw.table.SimpleTable.makeMinimalSchema and lsst.afw.table.Schema , but you get the gist.) This goes significantly beyond the immediate scope of this issue, though, so I don't think it has to be done before this is merged. (Indeed, I'm not even sure if I'm allowed to request it here, or if I should open a new issue...)
            Hide
            jbosch Jim Bosch added a comment -

            I think it's normally best to put requests for large changes that are beyond the scope of the original issue on a new issue (small change requests are fine). And it's also allowed for reviewees to take what they feel is a too-large change request and make a new issue instead of addressing it immediately. I'll leave it to Perry to decide whether to do your suggested cleanup here, or push it to a new issue, as I think it falls somewhere in the middle.

            And since Perry might not know the answer off the top of his head, I can confirm that writeFits will clobber, and I do think that's the desired behavior. I personally like leaving the files around for forensics after a test failure, but I'm not certain that opinion is unanimous.

            Show
            jbosch Jim Bosch added a comment - I think it's normally best to put requests for large changes that are beyond the scope of the original issue on a new issue (small change requests are fine). And it's also allowed for reviewees to take what they feel is a too-large change request and make a new issue instead of addressing it immediately. I'll leave it to Perry to decide whether to do your suggested cleanup here, or push it to a new issue, as I think it falls somewhere in the middle. And since Perry might not know the answer off the top of his head, I can confirm that writeFits will clobber, and I do think that's the desired behavior. I personally like leaving the files around for forensics after a test failure, but I'm not certain that opinion is unanimous.
            Hide
            pgee Perry Gee added a comment -

            I queried Robin on whether finally statements should be included in tests which create files, and her response is that leaving the test files around on failure is desirable.

            This isn't my test, and the only thing I did when I read it was to look for name with test.fits and remove them. However, I agree that using the same filename three times is a bad idea, even though it is actually OK to destroy each file once the test has passed. But the lack of unique names is confusing. So I will fix that, but not restructure the code.

            If this is OK with you, please mark Review Complete so that it will come back to me.

            Show
            pgee Perry Gee added a comment - I queried Robin on whether finally statements should be included in tests which create files, and her response is that leaving the test files around on failure is desirable. This isn't my test, and the only thing I did when I read it was to look for name with test.fits and remove them. However, I agree that using the same filename three times is a bad idea, even though it is actually OK to destroy each file once the test has passed. But the lack of unique names is confusing. So I will fix that, but not restructure the code. If this is OK with you, please mark Review Complete so that it will come back to me.
            Hide
            swinbank John Swinbank added a comment -

            Ok; agreed that further restructuring is outside the narrow scope of this issue.

            Show
            swinbank John Swinbank added a comment - Ok; agreed that further restructuring is outside the narrow scope of this issue.

              People

              • Assignee:
                pgee Perry Gee
                Reporter:
                robyn Robyn Allsman [X] (Inactive)
                Reviewers:
                John Swinbank
                Watchers:
                Jim Bosch, John Swinbank, Perry Gee, Robert Lupton, Robyn Allsman [X] (Inactive)
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel