Skip to content

Remove assert in production code#728

Merged
coretl merged 102 commits intobluesky:mainfrom
Canadian-Light-Source:710_remove_assert
Jan 15, 2025
Merged

Remove assert in production code#728
coretl merged 102 commits intobluesky:mainfrom
Canadian-Light-Source:710_remove_assert

Conversation

@kivel
Copy link
Contributor

@kivel kivel commented Jan 10, 2025

The changes will remove assert statements and replace them with check/raise.
Logic should remain identical, except for velocity related assertions in the timeout calculation.

new ruff rule

rule S101 was added to avoid future use of assert

Tango

There are a few remaining uses of assert in the Tango code. I ask for a Tango developer to take a look at those. If fail understand why they are used and also struggle with seemingly unrelated tests failing when I touch them.

assert in general

I understand that a simple assert foo.bar will make language support tools happy, as it is certain that bar exists for subsequent code. There are alternatives, admittedly, not as straight forward, though.
The general question for code hygiene should be, what comes first, a happy type checker or readable code. It happened a few time in my quest to remove the assert statements that it was unclear to me, why assert was used in the first place. I'd at least add a comment, if assert is used to make Pylance happy.

velocity checks

Previously, the velocity needed to be positive. The error message was stating however that the velocity is zero. Since the EPICS motorRecord will work with both, the check was changed to specifically check for zero. Technically, the check isn't even needed, as the subsequent timeout calculation will throw a DivisionByZero exception. This is of course, in the case we allow negative velocities ... which we should.

kivel added 30 commits January 7, 2025 15:27
strict check for ZERO velocity, sim Mover and epics Motor
minor refactor to reduce nesting in more linear flow with `elif`
- added TypeGaurd to make Pylance happy.
- added TypeError raised test
@kivel
Copy link
Contributor Author

kivel commented Jan 13, 2025

@coretl I addressed your requests to my abilities.

@coretl
Copy link
Collaborator

coretl commented Jan 14, 2025

Please could you resolve the conflicts and remove that commented out code and then I will merge

@kivel
Copy link
Contributor Author

kivel commented Jan 14, 2025

Please could you resolve the conflicts and remove that commented out code and then I will merge
sure!

merged main into feature and fixed all conflicts 34e0b70

@coretl
Copy link
Collaborator

coretl commented Jan 14, 2025

Looks like it still has conflicts? Also, lint seems to be failing...

@kivel
Copy link
Contributor Author

kivel commented Jan 14, 2025

I have absolutely no idea why the ruff format step in the workflow insists on reformatting the 4 files and make it fail.
It looks to me as if a different formatting rule is used in the workflow. All the tools work on my host in the provide dev-container

(venv) root ➜ /workspaces/ophyd-async (710_remove_assert) $ ruff format
178 files left unchanged
(venv) root ➜ /workspaces/ophyd-async (710_remove_assert) $ pre-commit run -a
check for added large files..............................................Passed
check yaml...............................................................Passed
check for merge conflicts................................................Passed
fix end of files.........................................................Passed
lint with ruff...........................................................Passed
format with ruff.........................................................Passed
Ensure import directionality.............................................Passed
(venv) root ➜ /workspaces/ophyd-async (710_remove_assert) $ git status
On branch 710_remove_assert
Your branch is up to date with 'origin/710_remove_assert'.

nothing to commit, working tree clean

@coretl
Copy link
Collaborator

coretl commented Jan 15, 2025

I have absolutely no idea why the ruff format step in the workflow insists on reformatting the 4 files and make it fail.

I think that is because ruff changed its formatting rules (looks like it does that once a year): https://github.com/astral-sh/ruff/releases/tag/0.9.0

If you run pip install ruff --upgrade in your venv then ruff format then it should match CI

@kivel
Copy link
Contributor Author

kivel commented Jan 15, 2025

@burkeds Thanks for looking into the Tango part.

@coretl Thanks for pointing out the need to update ruff. That made the CI work.
That should conclude this PR.

@coretl coretl merged commit a59ddcf into bluesky:main Jan 15, 2025
14 of 15 checks passed
paula-mg pushed a commit that referenced this pull request Mar 26, 2025
* add S101 (assert) to ruff and exclude all test code directories

* assert removed from plan_stubs

* test raise for setup_ndstats

* assert removed from fastcs panda control

* typo

* assert removed from fastcs table

* assert removed from epics sim Mover

* assert removed from epics motor
strict check for ZERO velocity, sim Mover and epics Motor

* assert removed from epics core p4p and aicoa
minor refactor to reduce nesting in more linear flow with `elif`

* assert removed from epics adsimdetector

* assert removed from epics adcore hdf_writer

* assert removed from TangoMover

* allow assert in tango transport for now

* assert removed from PatternDetectorController

* assert removed from PatternDetector write_data_to_dataset

* assert removed from PatternDetector open_file

* assert removed from PatternDetector collect_stream_docs

* assert removed from soft_signal_backend

* assert removed from SignalR._backend_or_chache

* assert removed from _SignalCache.get_reading

* assert removed from _SignalCache._notify

* assert removed from Table

* fix imports

* assert removed from device_filler

* assert removed from StandardReadable.hints

* assert removed from StandardReadable.add_readables
- added TypeGaurd to make Pylance happy.
- added TypeError raised test

* assert removed from Device.connect connector check

* assert removed from Device.connect task check

* assert removed from DeviceVector.__setitem__

* assert removed from DeviceProcessor._caller_locals

* add tests for StandardDetector.prepare

* assert removed from StandardDetector.prepare

* add tests for StandardDetector.prepare

* assert removed from StandardDetector.trigger

* assert removed from StandardDetector.complete
added method to ensure existence of TriggerInfo.

* cleaned up unused imports

* more freedom in examples

* add S101 (assert) to ruff and exclude all test code directories

* assert removed from plan_stubs

* test raise for setup_ndstats

* assert removed from fastcs panda control

* typo

* assert removed from fastcs table

* assert removed from epics sim Mover

* assert removed from epics motor
strict check for ZERO velocity, sim Mover and epics Motor

* assert removed from epics core p4p and aicoa
minor refactor to reduce nesting in more linear flow with `elif`

* assert removed from epics adsimdetector

* assert removed from TangoMover

* allow assert in tango transport for now

* assert removed from PatternDetectorController

* assert removed from PatternDetector write_data_to_dataset

* assert removed from PatternDetector open_file

* assert removed from PatternDetector collect_stream_docs

* assert removed from soft_signal_backend

* assert removed from SignalR._backend_or_chache

* assert removed from _SignalCache.get_reading

* assert removed from _SignalCache._notify

* assert removed from Table

* fix imports

* assert removed from device_filler

* assert removed from StandardReadable.hints

* assert removed from StandardReadable.add_readables
- added TypeGaurd to make Pylance happy.
- added TypeError raised test

* assert removed from Device.connect connector check

* assert removed from Device.connect task check

* assert removed from DeviceVector.__setitem__

* assert removed from DeviceProcessor._caller_locals

* add tests for StandardDetector.prepare

* assert removed from StandardDetector.prepare

* add tests for StandardDetector.prepare

* assert removed from StandardDetector.trigger

* assert removed from StandardDetector.complete
added method to ensure existence of TriggerInfo.

* cleaned up unused imports

* more freedom in examples

* test case fixed for StandardDetector

* test removed due to upstream changes

* test ficed due to migration from hdf to fileio in upstream method

* test fixed to account for upstream changes

* assert removed from internal trigger in ADBaseController.prepare

* assert removed from  ADWriter

* assert allowed in TestingIOC as it's only for static type checking

* typo in src/ophyd_async/plan_stubs/_nd_attributes.py

Co-authored-by: Tom C (DLS) <101418278+coretl@users.noreply.github.com>

* re-order to have the happy path with priority

* fix type in test

* TypeGuard simplified

* explicit check for not None in src/ophyd_async/core/_signal.py

Co-authored-by: Tom C (DLS) <101418278+coretl@users.noreply.github.com>

* todo removed

* fixed broken timeout calc when timeout is provided

* fixed broken timeout in sim mover

* make function private

* check and return DeviceAnnotation

* use returned DeviceAnnotation after check

* move to free function

* moved triggerInfo check to free function

* added _ensure_reading to SignalCache

* added missing type hints to SignalCache

* removed commented code

* format fixes

* upgrade ruff version and reformat

* use same logic Tagno as for aioca and p4p callbacks

---------

Co-authored-by: Tom C (DLS) <101418278+coretl@users.noreply.github.com>
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.

3 participants