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

Fix bug in OBS obs_stop for cmd_evt maneuver and improve default_stop handling #319

Merged
merged 4 commits into from
Feb 15, 2024

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Feb 9, 2024

Description

OBS obs_stop

This fixes a bug where an OBS statement coming from command events (during anomaly recovery) could have the wrong obs_stop value.

When an OBS statement is created it uses the time of the next maneuver or schedule stop to set obs_stop. In normal processing those are always available, but for a maneuver in anomaly recovery neither are available and it uses a default 7 days in the future.

Normally commands which change in any way by subsequent events or loads are detecting by the diff algorithm and replace. However the obs_stop update is just part of the command params and this is not checked in the diffing. Doing so introduces false diffs due to small floating point precision issues. So this PR special-cases OBS so that the params are included in the match key.

KADI_COMMANDS_DEFAULT_STOP

An unrelated issue surfaced during testing that the KADI_COMMANDS_DEFAULT_STOP environment variable was not being applied as a filter to events the command events sheet. This was unexpected to me and would make it very difficult to do detailed "scenario testing" of stepping through the timeline of past events. In this case having the full history of command events meant that the code always had subsequent maneuvers so seeing the OBS problem was difficult.

Interface impacts

None

Testing

Unit tests

  • Mac
(ska3) ➜  kadi git:(fix-matching-for-obs-cmds) git rev-parse HEAD  
3301432d93e8dbd79418873e076abab68a1a9170
(ska3) ➜  kadi git:(fix-matching-for-obs-cmds) pytest
=========================================================== test session starts ===========================================================
platform darwin -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/aldcroft/git, configfile: pytest.ini
plugins: timeout-2.1.0, anyio-3.6.2
collected 232 items                                                                                                                       

kadi/commands/tests/test_commands.py ......................................................................................         [ 37%]
kadi/commands/tests/test_states.py .......................x..............................................x.......................   [ 77%]
kadi/commands/tests/test_validate.py ....................                                                                           [ 86%]
kadi/tests/test_events.py ..........                                                                                                [ 90%]
kadi/tests/test_occweb.py ......................                                                                                    [100%]

============================================================ warnings summary =============================================================
kadi/kadi/commands/tests/test_commands.py::test_get_starcats_obsid
  /Users/aldcroft/miniconda3/envs/ska3/lib/python3.10/site-packages/mica/utils.py:16: FutureWarning: ska_load_dir() is deprecated. Use parse_cm.paths.load_dir_from_load_name()
    dir_parts = ska_load_dir(load_name).parts

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
========================================== 230 passed, 2 xfailed, 1 warning in 99.43s (0:01:39) ===========================================

Independent check of unit tests by Jean

  • OSX
(ska3) flame:kadi jean$ pytest
===================================================================== test session starts =====================================================================
platform darwin -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/jean/git, configfile: pytest.ini
plugins: timeout-2.1.0, anyio-3.6.2
collected 232 items                                                                                                                                           

kadi/commands/tests/test_commands.py .......s.......s.....................................................................s                             [ 37%]
kadi/commands/tests/test_states.py .......................x..............................................x.......................                       [ 77%]
kadi/commands/tests/test_validate.py ....................                                                                                               [ 86%]
kadi/tests/test_events.py ..........                                                                                                                    [ 90%]
kadi/tests/test_occweb.py ......................                                                                                                        [100%]

====================================================================== warnings summary =======================================================================
kadi/kadi/commands/tests/test_commands.py::test_get_starcats_each_year[year0]
  /Users/jean/miniconda3/envs/ska3/lib/python3.10/site-packages/setuptools_scm/git.py:295: UserWarning: git archive did not support describe output
    warnings.warn("git archive did not support describe output")

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=============================================== 227 passed, 3 skipped, 2 xfailed, 1 warning in 81.41s (0:01:21) ===============================================
(ska3) flame:kadi jean$ git rev-parse HEAD
3301432d93e8dbd79418873e076abab68a1a9170

Functional tests

Detailed functional testing in validate/pr319. This demonstrates that the patch fixes the problem. This functional validation should work on HEAD.

# I did this before committing the script. To replicate copy the script to a different name.
git switch master  
python run_updates.py > master.log
git switch fix-matching-for-obs-cmds
python run_updates.py > fix-matching-for-obs-cmds.log

@taldcroft taldcroft changed the title WIP: Fix a bug in OBS obs_stop for last cmd_evt maneuver Fix bug in OBS obs_stop for cmd_evt maneuver and fix default_stop handling Feb 11, 2024
@taldcroft taldcroft changed the title Fix bug in OBS obs_stop for cmd_evt maneuver and fix default_stop handling Fix bug in OBS obs_stop for cmd_evt maneuver and improve default_stop handling Feb 11, 2024
@jeanconn
Copy link
Contributor

The only thing that niggles at me is that KADI_COMMANDS_DEFAULT_STOP is not really a default (if it has a value, we're doing something special that is non-default) but that is neither here nor there.

@taldcroft taldcroft merged commit 3885319 into master Feb 15, 2024
4 checks passed
@taldcroft taldcroft deleted the fix-matching-for-obs-cmds branch February 15, 2024 10:14
This was referenced Mar 6, 2024
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.

2 participants