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

migrate p38 to i22 #862

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

migrate p38 to i22 #862

wants to merge 3 commits into from

Conversation

stan-dot
Copy link
Contributor

Fixes #502

Instructions to reviewer on how to test:

  1. BEAMLINE=p38 dodal connect i22
  2. expect all to connect

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}

@stan-dot stan-dot added enhancement New feature or request python Pull requests that update Python code low priority Not needed for production in the near future i22 labels Oct 24, 2024
@stan-dot stan-dot self-assigned this Oct 24, 2024
@stan-dot
Copy link
Contributor Author

@pytest.fixture(scope="function")
def module_and_devices_for_beamline(request):
    beamline = request.param
    with patch.dict(os.environ, {"BEAMLINE": beamline}, clear=True):
        bl_mod = importlib.import_module("dodal.beamlines." + beamline)
        importlib.reload(bl_mod)
        mock_beamline_module_filepaths(beamline, bl_mod)
        devices, _ = make_all_devices(
            bl_mod,
            include_skipped=True,
            fake_with_ophyd_sim=True,
        )
        yield (bl_mod, devices)
        beamline_utils.clear_devices()
        del bl_mod

the conftest is very opinionated and overriding it 'from the other file' seems impossible,

the quick and easy way to fix is it to use another conftest,

or add some param to the fixture to skip the os environ patch if it's a lab beamline

@DominicOram , @callumforrester please share your perspectives

Comment on lines +36 to +38
f"/dls/${'p38' if IS_LAB else 'i22'}/data/2024/cm37271-2/bluesky"
)
API_URL: str = f"http://{'p38' if IS_LAB else 'i22'}-control:8088/api"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should: I think BL contains p38 or i22 string can't we use that rather than the if statement here?

@DominicOram
Copy link
Contributor

the conftest is very opinionated and overriding it 'from the other file' seems impossible,

Sorry, I'm not sure I understand. You can override the BEAMLINE env variable by passing in the beamline you want like in @pytest.mark.parametrize("module_and_devices_for_beamline", ["i22"], indirect=True). Or are you trying to override something else?

On a side-note I would be careful about merging this until we have fixed #864 as it could be masking errors.

@@ -25,10 +25,18 @@
from dodal.utils import BeamlinePrefix, get_beamline_name

BL = get_beamline_name("i22")
IS_LAB = BL == "p38"
LINKAM_IS_IN_LAB = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Should: Our current design for beamlines where some equipment may or may not be there is to try to initialise every device, report the failures and move on, so we should always try to connect the linkam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request i22 low priority Not needed for production in the near future python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically keep I22 and P38 in sync
3 participants