Skip to content

Conversation

@noemifrisina
Copy link
Collaborator

Renames src/dodal/plans/scanspec.py to avoid issues from the filename clashing with the package.

eg.

  File "/scratch/uhz96441/test/dodal/src/dodal/plans/save_panda.py", line 10, in <module>
    from ophyd_async.plan_stubs import (
  File "/scratch/uhz96441/test/dodal/.venv/lib/python3.11/site-packages/ophyd_async/plan_stubs/__init__.py", line 5, in <module>
    from ._panda import apply_panda_settings
  File "/scratch/uhz96441/test/dodal/.venv/lib/python3.11/site-packages/ophyd_async/plan_stubs/_panda.py", line 4, in <module>
    from ophyd_async.fastcs import panda
  File "/scratch/uhz96441/test/dodal/.venv/lib/python3.11/site-packages/ophyd_async/fastcs/panda/__init__.py", line 23, in <module>
    from ._trigger import (
  File "/scratch/uhz96441/test/dodal/.venv/lib/python3.11/site-packages/ophyd_async/fastcs/panda/_trigger.py", line 9, in <module>
    from scanspec.core import Path
  File "/scratch/uhz96441/test/dodal/src/dodal/plans/scanspec.py", line 9, in <module>
    from scanspec.specs import Spec
ModuleNotFoundError: No module named 'scanspec.specs'; 'scanspec' is not a package

Instructions to reviewer on how to test:

  1. Run tests and confirm they all still pass

Checks for reviewer

  • Would the PR title make sense to a scientist on a set of release notes
  • If a new device has been added does it follow the standards
  • If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
  • Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

@noemifrisina noemifrisina requested a review from a team as a code owner October 17, 2025 12:47
@codecov
Copy link

codecov bot commented Oct 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.90%. Comparing base (3277c91) to head (4049671).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1640   +/-   ##
=======================================
  Coverage   98.90%   98.90%           
=======================================
  Files         270      270           
  Lines        9888     9888           
=======================================
  Hits         9780     9780           
  Misses        108      108           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Can we have a test that would have caught this? I'm not sure off the top of my head how to reproduce it in a test but we should have a go

@DiamondJoseph
Copy link
Contributor

Thanks. Can we have a test that would have caught this? I'm not sure off the top of my head how to reproduce it in a test but we should have a go

I was thinking possibly a linting rule, but can't find it in the official ruff rules- it's been implemented in this third party additional ruleset: https://github.com/gforcada/flake8-builtins as A005, may give more to search on

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to find a better name for this file, it's not really reading a scanspec, the scanspec defines the trajectory of the motors.

@noemifrisina
Copy link
Collaborator Author

Thanks. Can we have a test that would have caught this? I'm not sure off the top of my head how to reproduce it in a test but we should have a go

Sorry, this one slipped my mind. I think I agree it should me more a linting rule than a test... Considering the problem came from the file being called the same as a package we're importing I'm not really sure we could come up with a test that checks this

Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this one slipped my mind. I think I agree it should me more a linting rule than a test...

I don't think it's a linting rule if you found a situation that raised an Exception in prod. We should be able to reproduce the error in a test. How do I reproduce getting the error described in this PR?

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.

4 participants