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

Remove internal parallelization from DefineVisitsTask and gen2to3

    XMLWordPrintable

    Details

      Description

      The internal parallelization in these obs_base tasks ranges from non-functional to not useful. It's time to just remove it.

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Note that parallelization is used in ap_verify ingestion:

            Since the parallelization in ap_verify goes all the way up to the shared CLI between ap_verify.py and ingest_dataset.py, I'd recommend just removing it from these two run calls instead of trying to make everything self-consistent. The ap_verify CLI reference does not explicitly say that ingestion is parallelized, after all...

            Show
            krzys Krzysztof Findeisen added a comment - - edited Note that parallelization is used in ap_verify ingestion: https://github.com/lsst/ap_verify/blob/main/python/lsst/ap/verify/ingestion.py#L185 https://github.com/lsst/ap_verify/blob/main/python/lsst/ap/verify/ingestion.py#L214 Since the parallelization in ap_verify goes all the way up to the shared CLI between ap_verify.py and ingest_dataset.py , I'd recommend just removing it from these two run calls instead of trying to make everything self-consistent. The ap_verify CLI reference does not explicitly say that ingestion is parallelized, after all...
            Hide
            tjenness Tim Jenness added a comment -

            We decided on Slack that the parallelization in raw ingest is still a reasonable thing to do since it parallelizes file metadata extraction and does not parallelize the butler registry side of things.

            Show
            tjenness Tim Jenness added a comment - We decided on Slack that the parallelization in raw ingest is still a reasonable thing to do since it parallelizes file metadata extraction and does not parallelize the butler registry side of things.
            Hide
            jbosch Jim Bosch added a comment -

            Ok, done, with parallelization removed from DefineVisitsTask and its scripts only.  I did the absolute minimal things to update gen2to3 and ap_verify, just not passing "processes" or "pool" on to DefineVisitsTask.run.  Krzysztof Findeisen , I'd be happy to do more in ap_verify if you'd like, but I think what I've done matches your recommendation.

            https://github.com/lsst/obs_base/pull/407

            https://github.com/lsst/ap_verify/pull/152

             

            Show
            jbosch Jim Bosch added a comment - Ok, done, with parallelization removed from DefineVisitsTask and its scripts only.  I did the absolute minimal things to update gen2to3 and ap_verify, just not passing "processes" or "pool" on to DefineVisitsTask.run.  Krzysztof Findeisen , I'd be happy to do more in ap_verify if you'd like, but I think what I've done matches your recommendation. https://github.com/lsst/obs_base/pull/407 https://github.com/lsst/ap_verify/pull/152  
            Hide
            tjenness Tim Jenness added a comment -

            Looks okay although I think the coding standard prefers we don't use map().

            Show
            tjenness Tim Jenness added a comment - Looks okay although I think the coding standard prefers we don't use map() .
            Hide
            jbosch Jim Bosch added a comment -

            I've removed the map usage, and that led to some nice other cleanups.  I also added a trivial PR for ci_builder (machinery behind ci_imsim), following a Jenkins failure:

            https://github.com/lsst-dm/ci_builder/pull/8

            I plan to merge after Jenkins is green.

            Show
            jbosch Jim Bosch added a comment - I've removed the map usage, and that led to some nice other cleanups.  I also added a trivial PR for ci_builder (machinery behind ci_imsim ), following a Jenkins failure: https://github.com/lsst-dm/ci_builder/pull/8 I plan to merge after Jenkins is green.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Tim Jenness
              Watchers:
              Jim Bosch, Krzysztof Findeisen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.