JP-2675: Bug in setting SKIPPED for cal_step keywords, when input is ModelContainer #62

tapastro · 2022-07-13T23:43:46Z

Addresses #61
Addresses JP-2675

Recent code added by yours truly tried to catch errors in meta keyword assignment of cal_step values to SKIPPED, but missed the possibliity of the first step in a pipeline being skipped, exposing this code to a ModelContainer. Code is added to "broadcast" behavior initially applied to a single input DataModel to all DataModel enclosed within the ModelContainer.

NB: This reinforces the need for an stdatamodels issue to add a parent class to ModelContainer that provides for a robust isinstance() check, e.g. @stscieisenhamer's suggested ModelList.

codecov · 2022-07-13T23:47:22Z

Codecov Report

Merging #62 (8c51f1b) into master (27b4430) will not change coverage.
The diff coverage is 0.00%.

@@          Coverage Diff           @@
##           master     #62   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          25      25           
  Lines        3060    3067    +7     
======================================
- Misses       3060    3067    +7

Impacted Files	Coverage Δ
src/stpipe/step.py	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27b4430...8c51f1b. Read the comment docs.

hbushouse

LGTM

hbushouse · 2022-07-14T00:28:11Z

Have you been able to do some semi-comprehensive testing on a range of use cases?

tapastro · 2022-07-14T00:44:33Z

Have you been able to do some semi-comprehensive testing on a range of use cases?

Full disclosure: I made this PR and left for dinner. I'll start a regression set against it, but besides testing on David's example data, it's not clear we have something to catch this. I'm also not convinced it warrants a test of its own, but this PR may suggest otherwise...

hbushouse · 2022-07-14T11:29:22Z

@ddavis-stsci is it possible for someone to test this PR against Roman before we merge it?

zacharyburnett · 2022-07-14T14:30:12Z

I ran regtests in Romancal against this PR from a clean environment:

pip install -e ".[test]" pytest-xdist
pip install git+https://github.com/tapastro/stpipe@jp-2675-cal-step-skip-bug
export CRDS_SERVER_URL=https://roman-crds-test.stsci.edu
pytest -n auto --bigdata romancal/regtest

and had the following failures:

`test_level2_image_processing_pipeline`

----------------------------------------------------------------------
ERROR PARSING CONFIGURATION:
    'roman_elp' is not a path to a config file or a Python Step class
----------------------------------------------------------------------
usage: -c [--logcfg LOGCFG] [--verbose] [--debug]
          [--save-parameters SAVE_PARAMETERS] [--disable-crds-steppars]
          cfg_file_or_class

Run an stpipe Step or Pipeline

positional arguments:
  cfg_file_or_class     The configuration file or Python class to run

options:
  --logcfg LOGCFG       The logging configuration file to load
  --verbose, -v         Turn on all logging messages
  --debug               When an exception occurs, invoke the Python debugger,
                        pdb
  --save-parameters SAVE_PARAMETERS
                        Save step parameters to specified file.
  --disable-crds-steppars
                        Disable retrieval of step parameter references files
                        from CRDS

romancal/regtest/test_wfi_pipeline.py:20 (test_level2_image_processing_pipeline)
identifier = 'roman_elp'

    def _get_config_and_class(identifier):
        """
        Given a file path to a config file or Python module path, return a
        Step class and a configuration object.
        """
        if os.path.exists(identifier):
            config_file = identifier
            config = config_parser.load_config_file(config_file)
            step_class, name = Step._parse_class_and_name(
                config, config_file=config_file)
        else:
            try:
>               step_class = utilities.import_class(utilities.resolve_step_class_alias(identifier), Step)

/opt/miniconda3/envs/romancal_20220714_JP-2675/lib/python3.10/site-packages/stpipe/cmdline.py:40: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

full_name = 'roman_elp', subclassof = <class 'stpipe.step.Step'>
config_file = None

    def import_class(full_name, subclassof=object, config_file=None):
        """
        Import the Python class `full_name` given in full Python package format,
        e.g.::
    
            package.another_package.class_name
    
        Return the imported class. Optionally, if `subclassof` is not None
        and is a Python class, make sure that the imported class is a
        subclass of `subclassof`.
        """
        # Understand which class we need to instantiate. The class name is given in
        # full Python package notation, e.g.
        #   package.subPackage.subsubpackage.className
        # in the input parameter `full_name`. This means that
        #   1. We HAVE to be able to say
        #       from package.subPackage.subsubpackage import className
        #   2. If `subclassof` is defined, the newly imported Python class MUST be a
        #      subclass of `subclassof`, which HAS to be a Python class.
    
        if config_file is not None:
            sys.path.insert(0, os.path.dirname(config_file))
    
        try:
            full_name = full_name.strip()
            package_name, sep, class_name = full_name.rpartition('.')
            if not package_name:
>               raise ImportError("{0} is not a Python class".format(full_name))
E               ImportError: roman_elp is not a Python class

/opt/miniconda3/envs/romancal_20220714_JP-2675/lib/python3.10/site-packages/stpipe/utilities.py:59: ImportError

During handling of the above exception, another exception occurred:

rtdata = {'input': '/private/var/folders/bv/nnh6jr1553jcv2pxbt8hyw6c00053_/T/pytest-of-zburnett/pytest-3/popen-gw6/test_level2_...cal.asdf',
 'remote_results_path': None,
 'test_name': None,
 'traceback': None,
 'truth': None,
 'truth_remote': None}
ignore_asdf_paths = {'ignore': ['meta.[date, filename]', 'asdf_library', 'history', 'cal_logs']}

    @pytest.mark.bigdata
    @pytest.mark.soctests
    def test_level2_image_processing_pipeline(rtdata, ignore_asdf_paths):
        """Tests for flat field imaging processing requirements DMS86 & DMS 87 """
        input_data = "r0000101001001001001_01101_0001_WFI01_uncal.asdf"
        rtdata.get_data(f"WFI/image/{input_data}")
        rtdata.input = input_data
    
        # Test Pipeline
        output = "r0000101001001001001_01101_0001_WFI01_cal.asdf"
        rtdata.output = output
        args = ["--disable-crds-steppars",
                "--steps.jump.rejection_threshold=180.0",
                "--steps.jump.three_group_rejection_threshold=185.0",
                "--steps.jump.four_group_rejection_threshold=190",
                "roman_elp", rtdata.input,
                ]
>       ExposurePipeline.from_cmdline(args)

/Users/zburnett/projects/romancal/romancal/regtest/test_wfi_pipeline.py:38: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/opt/miniconda3/envs/romancal_20220714_JP-2675/lib/python3.10/site-packages/stpipe/step.py:179: in from_cmdline
    return cmdline.step_from_cmdline(args)
/opt/miniconda3/envs/romancal_20220714_JP-2675/lib/python3.10/site-packages/stpipe/cmdline.py:341: in step_from_cmdline
    just_the_step_from_cmdline(args, cls)
/opt/miniconda3/envs/romancal_20220714_JP-2675/lib/python3.10/site-packages/stpipe/cmdline.py:198: in just_the_step_from_cmdline
    _get_config_and_class(known.cfg_file_or_class[0])
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

identifier = 'roman_elp'

    def _get_config_and_class(identifier):
        """
        Given a file path to a config file or Python module path, return a
        Step class and a configuration object.
        """
        if os.path.exists(identifier):
            config_file = identifier
            config = config_parser.load_config_file(config_file)
            step_class, name = Step._parse_class_and_name(
                config, config_file=config_file)
        else:
            try:
                step_class = utilities.import_class(utilities.resolve_step_class_alias(identifier), Step)
            except (ImportError, AttributeError, TypeError):
>               raise ValueError(
                    '{0!r} is not a path to a config file or a Python Step '
                    'class'.format(identifier))
E               ValueError: 'roman_elp' is not a path to a config file or a Python Step class

/opt/miniconda3/envs/romancal_20220714_JP-2675/lib/python3.10/site-packages/stpipe/cmdline.py:42: ValueError

`test_level2_grism_processing_pipeline`

----------------------------------------------------------------------
ERROR PARSING CONFIGURATION:
    'roman_elp' is not a path to a config file or a Python Step class
----------------------------------------------------------------------
usage: -c [--logcfg LOGCFG] [--verbose] [--debug]
          [--save-parameters SAVE_PARAMETERS] [--disable-crds-steppars]
          cfg_file_or_class

Run an stpipe Step or Pipeline

positional arguments:
  cfg_file_or_class     The configuration file or Python class to run

options:
  --logcfg LOGCFG       The logging configuration file to load
  --verbose, -v         Turn on all logging messages
  --debug               When an exception occurs, invoke the Python debugger,
                        pdb
  --save-parameters SAVE_PARAMETERS
                        Save step parameters to specified file.
  --disable-crds-steppars
                        Disable retrieval of step parameter references files
                        from CRDS

romancal/regtest/test_wfi_pipeline.py:170 (test_level2_grism_processing_pipeline)
identifier = 'roman_elp'

    def _get_config_and_class(identifier):
        """
        Given a file path to a config file or Python module path, return a
        Step class and a configuration object.
        """
        if os.path.exists(identifier):
            config_file = identifier
            config = config_parser.load_config_file(config_file)
            step_class, name = Step._parse_class_and_name(
                config, config_file=config_file)
        else:
            try:
>               step_class = utilities.import_class(utilities.resolve_step_class_alias(identifier), Step)

/opt/miniconda3/envs/romancal_20220714_JP-2675/lib/python3.10/site-packages/stpipe/cmdline.py:40: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

full_name = 'roman_elp', subclassof = <class 'stpipe.step.Step'>
config_file = None

    def import_class(full_name, subclassof=object, config_file=None):
        """
        Import the Python class `full_name` given in full Python package format,
        e.g.::
    
            package.another_package.class_name
    
        Return the imported class. Optionally, if `subclassof` is not None
        and is a Python class, make sure that the imported class is a
        subclass of `subclassof`.
        """
        # Understand which class we need to instantiate. The class name is given in
        # full Python package notation, e.g.
        #   package.subPackage.subsubpackage.className
        # in the input parameter `full_name`. This means that
        #   1. We HAVE to be able to say
        #       from package.subPackage.subsubpackage import className
        #   2. If `subclassof` is defined, the newly imported Python class MUST be a
        #      subclass of `subclassof`, which HAS to be a Python class.
    
        if config_file is not None:
            sys.path.insert(0, os.path.dirname(config_file))
    
        try:
            full_name = full_name.strip()
            package_name, sep, class_name = full_name.rpartition('.')
            if not package_name:
>               raise ImportError("{0} is not a Python class".format(full_name))
E               ImportError: roman_elp is not a Python class

/opt/miniconda3/envs/romancal_20220714_JP-2675/lib/python3.10/site-packages/stpipe/utilities.py:59: ImportError

During handling of the above exception, another exception occurred:

rtdata = {'input': '/private/var/folders/bv/nnh6jr1553jcv2pxbt8hyw6c00053_/T/pytest-of-zburnett/pytest-3/popen-gw7/test_level2_...cal.asdf',
 'remote_results_path': None,
 'test_name': None,
 'traceback': None,
 'truth': None,
 'truth_remote': None}
ignore_asdf_paths = {'ignore': ['meta.[date, filename]', 'asdf_library', 'history', 'cal_logs']}

    @pytest.mark.bigdata
    @pytest.mark.soctests
    def test_level2_grism_processing_pipeline(rtdata, ignore_asdf_paths):
        """Tests for flat field grism processing requirements DMS90 & DMS 91 """
        input_data = "r0000201001001001002_01101_0001_WFI01_uncal.asdf"
        rtdata.get_data(f"WFI/grism/{input_data}")
        rtdata.input = input_data
    
        # Test Pipeline
        output = "r0000201001001001002_01101_0001_WFI01_cal.asdf"
        rtdata.output = output
        args = ["--disable-crds-steppars",
                "--steps.jump.rejection_threshold=180.0",
                "--steps.jump.three_group_rejection_threshold=185.0",
                "--steps.jump.four_group_rejection_threshold=190",
                "roman_elp", rtdata.input,
                ]
>       ExposurePipeline.from_cmdline(args)

/Users/zburnett/projects/romancal/romancal/regtest/test_wfi_pipeline.py:188: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/opt/miniconda3/envs/romancal_20220714_JP-2675/lib/python3.10/site-packages/stpipe/step.py:179: in from_cmdline
    return cmdline.step_from_cmdline(args)
/opt/miniconda3/envs/romancal_20220714_JP-2675/lib/python3.10/site-packages/stpipe/cmdline.py:341: in step_from_cmdline
    just_the_step_from_cmdline(args, cls)
/opt/miniconda3/envs/romancal_20220714_JP-2675/lib/python3.10/site-packages/stpipe/cmdline.py:198: in just_the_step_from_cmdline
    _get_config_and_class(known.cfg_file_or_class[0])
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

identifier = 'roman_elp'

    def _get_config_and_class(identifier):
        """
        Given a file path to a config file or Python module path, return a
        Step class and a configuration object.
        """
        if os.path.exists(identifier):
            config_file = identifier
            config = config_parser.load_config_file(config_file)
            step_class, name = Step._parse_class_and_name(
                config, config_file=config_file)
        else:
            try:
                step_class = utilities.import_class(utilities.resolve_step_class_alias(identifier), Step)
            except (ImportError, AttributeError, TypeError):
>               raise ValueError(
                    '{0!r} is not a path to a config file or a Python Step '
                    'class'.format(identifier))
E               ValueError: 'roman_elp' is not a path to a config file or a Python Step class

/opt/miniconda3/envs/romancal_20220714_JP-2675/lib/python3.10/site-packages/stpipe/cmdline.py:42: ValueError

tapastro · 2022-07-14T17:08:50Z

To my eye, those error traces point to an issue occurring in cmdline, while this change is a pretty narrow keyword update change in step.py. Are those errors not present when tested on 0.4.0 or master?

zacharyburnett · 2022-07-14T17:20:58Z

To my eye, those error traces point to an issue occurring in cmdline, while this change is a pretty narrow keyword update change in step.py. Are those errors not present when tested on 0.4.0 or master?

You're right, I thought I had tested on master previously (and had green tests) but I had actually overrode that control test with the PyPI release on accident, so I was actually testing 0.4.0. When I reinstalled from master to the control environment I got the same failing tests; no difference in tests between master and this PR.

zacharyburnett · 2022-07-14T17:43:49Z

it seems, then, that there aren't any new failures in the regtests introduced by this PR from what I can see

hbushouse · 2022-07-14T17:46:06Z

So is that my green light to go ahead and merge this, and then have someone create a new stpipe release?

nden · 2022-07-14T17:59:39Z

I ran one romancal test with this branch (corrected for the problem in #54 and it passed.

zacharyburnett · 2022-07-14T18:00:51Z

So is that my green light to go ahead and merge this, and then have someone create a new stpipe release?

I would think so, but I'll defer to @ddavis-stsci and @nden. I'm currently testing against the 0.4.0 PyPI distribution, and it seems like all tests pass, so I'm thinking the failing regtests might just be an artifact of how pip install git+ installs stpipe.

nden · 2022-10-11T08:48:43Z

I ran successfully a Roman test.

…

On Thu, Jul 14, 2022, 6:29 AM Howard Bushouse ***@***.***> wrote: @ddavis-stsci <https://github.com/ddavis-stsci> is it possible for someone to test this PR against Roman before we merge it? — Reply to this email directly, view it on GitHub <#62 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIPCWNLSLILXUN4AY25DV3VT72ZZANCNFSM53QMZHHA> . You are receiving this because your review was requested.Message ID: ***@***.***>

do things differently for ModelContainers

8550a74

tapastro requested review from nden, hbushouse and ddavis-stsci July 13, 2022 23:43

tapastro added 2 commits July 13, 2022 19:44

changelog

96553e5

changelog typo

8c51f1b

hbushouse approved these changes Jul 14, 2022

View reviewed changes

nden approved these changes Jul 14, 2022

View reviewed changes

hbushouse merged commit e11963c into spacetelescope:master Jul 14, 2022

This was referenced Jul 14, 2022

update stpipe requirement to >=0.4.1 spacetelescope/jwst#6925

Merged

update stpipe requirement to >=0.4.1 spacetelescope/romancal#545

Merged

zacharyburnett added this to the 0.4.1 milestone Jul 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JP-2675: Bug in setting SKIPPED for cal_step keywords, when input is ModelContainer #62

JP-2675: Bug in setting SKIPPED for cal_step keywords, when input is ModelContainer #62

tapastro commented Jul 13, 2022

codecov bot commented Jul 13, 2022 •

edited

hbushouse left a comment

hbushouse commented Jul 14, 2022

tapastro commented Jul 14, 2022

hbushouse commented Jul 14, 2022

zacharyburnett commented Jul 14, 2022 •

edited

tapastro commented Jul 14, 2022

zacharyburnett commented Jul 14, 2022 •

edited

zacharyburnett commented Jul 14, 2022

hbushouse commented Jul 14, 2022

nden commented Jul 14, 2022

zacharyburnett commented Jul 14, 2022

nden commented Oct 11, 2022 via email

JP-2675: Bug in setting SKIPPED for cal_step keywords, when input is ModelContainer #62

JP-2675: Bug in setting SKIPPED for cal_step keywords, when input is ModelContainer #62

Conversation

tapastro commented Jul 13, 2022

codecov bot commented Jul 13, 2022 • edited

Codecov Report

hbushouse left a comment

Choose a reason for hiding this comment

hbushouse commented Jul 14, 2022

tapastro commented Jul 14, 2022

hbushouse commented Jul 14, 2022

zacharyburnett commented Jul 14, 2022 • edited

tapastro commented Jul 14, 2022

zacharyburnett commented Jul 14, 2022 • edited

zacharyburnett commented Jul 14, 2022

hbushouse commented Jul 14, 2022

nden commented Jul 14, 2022

zacharyburnett commented Jul 14, 2022

nden commented Oct 11, 2022 via email

codecov bot commented Jul 13, 2022 •

edited

zacharyburnett commented Jul 14, 2022 •

edited

zacharyburnett commented Jul 14, 2022 •

edited