Breaking: Redesign structure to use plugin structure by oliwenmandiamond · Pull Request #165 · DiamondLightSource/daq-config-server

oliwenmandiamond · 2026-03-03T14:50:58Z

Fixes #164

jacob720

To me, the name plugins is a bit less clear and specific then the name models. I also think it's slightly confusing having lots of _converters.py files outside of the converters directory but maybe there's no way around this.

Also, are you against having the option of doing

from daq_config_server.models import MyModel

As we could presumably keep the other improvements to the module structure but also add models to daq_config_sever.models.__init__.py, allowing you to import them in whichever way you prefer.

jacob720 · 2026-03-03T16:37:59Z

src/daq_config_server/converters/_file_converter_map.py

 import xmltodict

-from daq_config_server.models.converters.beamline_parameters import (
+from daq_config_server.core._base_model import ConfigModel


Can we avoid importing from private modules that are in different directories?
I think we should expose ConfigModel in daq_config_server.core.__init__.py

olliesilvester · 2026-03-03T17:12:54Z

To me, the name plugins is a bit less clear and specific then the name models

I was going to comment the same thing

oliwenmandiamond · 2026-03-03T17:29:15Z

Yeah, I can rename back to models, no problem

Can we avoid importing from private modules that are in different directories? I think we should expose ConfigModel in daq_config_server.core.__init__.py

Yeah, could maybe even just put it at the top of daq_config_server.models

Also, are you against having the option of doing
from daq_config_server.models import MyModel
As we could presumably keep the other improvements to the module structure but also add models to daq_config_sever.models.__init__.py, allowing you to import them in whichever way you prefer.

The point is NOT to share the same namespace as other models / plugins / whatever. They should be isolated and not depend on one another / have visibility of each other.

Otherwise, what's the point of the separate model structure if we then force them into the same namespace anyway? We might as well just put everything into a single massive file

jacob720 · 2026-03-03T18:09:31Z

Personally I don't think the import structure has to exactly reflect the internal file structure.

Otherwise, what's the point of the separate model structure if we then force them into the same namespace anyway? We might as well just put everything into a single massive file

This would make it horrible for developers for work on. The internal file structure to me is a separate decision to how we want to design the public interface. I have a preference for a fairly flat import structure and a more organised file structure, but not a strong one so happy to be overuled

oliwenmandiamond · 2026-03-04T09:42:48Z

Otherwise, what's the point of the separate model structure if we then force them into the same namespace anyway? We might as well just put everything into a single massive file

This would make it horrible for developers for work on.

Exactly! This is the core issue. You are trying to use both structures, so you remove the advantages of one way and then give it all the disadvantages of the other. One needs to be picked! Now you are indirectly causing them all share the the same namespace which can easily cause confusion and name conflicts.

For example, to make a comparison, lets take dodal.devices.beamlines. Each beamline has folder for it's beamline specific devices. Now lets apply whats been done to the daq-config-server to it

Instead of using from dodal.devices.beamlines.iXX import MyDevice, I now do from dodal.devices.beamlines import MyDevice. What on earth is MyDevice? What beamline is it from? Not only that, but now if any other beamline also wants to name their device class the same thing as another because they are in their own namespace and they need a slightly more specific thing and is independent of every other beamline and has no visibility of others, it can name it the same thing but then causes issues in the init.py file because of the name conflicts. If it was in the same file, the name conflict would be visible to the developer straight away rather than indirectly. Not only this, you also now have to maintain the imports in two places rather one, it is a maintenance nightmare!

As I mentioned previously, it may not seem like an issue right now because only really MX are using it. But if me and many other developers want to also use this service, this will quickly get out of hand.

olliesilvester · 2026-03-04T09:53:26Z

I think you're both actually almost on the same page here, we all agree that something like
from daq_config_server.models import MyModel
would be confusing in general. I would suggest:

Import different model modules into daq_config_server.models, so then for example your code using the library would do from daq_config_server.models import ixx_models
Also import generically useful converters, eg from daq_config_server.models import StandardLutConverter.

oliwenmandiamond · 2026-03-04T10:00:01Z

I think you're both actually almost on the same page here, we all agree that something like from daq_config_server.models import MyModel would be confusing in general. I would suggest:
* Import different model modules into  `daq_config_server.models`, so then for example your code using the library would do `from daq_config_server.models import ixx_models`

* Also import generically useful converters, eg `from daq_config_server.models import StandardLutConverter`.

Yes, having core / common models at the top level is perfectly fine, as long as that is where it actually lives. So it would look like this

models
     __init__.py
     _common_model.py
     _common_converter.py

     specific_thing1
          __init__.py
          _model.py
          _converter.py

     specific_thing2
          __init__.py
          _model.py
          _converter.py

imports are now

from daq_config_server.models import MyCommonModel
from daq_config_server.models.specific_thing1 import MySpecificModel
from daq_config_server.models.specific_thing2 import MySpecificModel

So common things from a quick glance would be GenericLookupTable, LookupTableBase

oliwenmandiamond · 2026-03-05T11:30:39Z

@olliesilvester @jacob720 I think this is ready for review now. Apologies on size of change.

The main clean up I've done is:

Clearly define packages app, models, converters, and testing. These are broken down further into their specific use cases so everything is well organised and makes sense navigating.
Models package focuses on models. Models now implement a from_contents class method. This simplifies logic and couples the loader with the model, making code easier to read and navigate. Where this isn't possible, the converter/loader function lives with the models as it is to do with loading a model.
Models are also reorganised, separating out ones which are generic and more specialised.
Tests reflect this new structure.

I will raise a separate issue so that external repositories can only import what they need, which is ConfigServer and the models package.

oliwenmandiamond · 2026-03-05T15:54:15Z

I'm also happy to go around all the external repos and apply the breaking change to them so it all rolls out smoothly

olliesilvester

Thanks, the structure is nicer I think. I haven't gone through all the tests and logic just yet

olliesilvester · 2026-03-05T15:50:48Z

src/daq_config_server/app/_convert.py

-from ._converter_utils import ConverterParseError
+import daq_config_server.converters._file_converter_map as file_converter_map
+from daq_config_server.converters._converter_utils import ConverterParseError
+from daq_config_server.models._base_model import ConfigModel


 def get_converted_file_contents(file_path: Path) -> dict[str, Any]:


Should: expose get_converted_file_contents in converters/__init__.py, so that other files don't have to import from this private file.

Putting something in __all__ signals that it is part of the public API and intended for external use. It is and should only be used inside this package I believe so doesn't make sense to.

I think you're maybe mixing up what's publicly available to those using daq-config-server as a library, and public/private parts internal to the repo

Since get_converted_file_contents is defined in the converters module but is used by the app module, we should indicate that get_converted_file_contents is okay to use in other modules. Same goes for my other comments

We could go even further and make another module that exposes the external API in a neater way. Maybe for a separate issue though

src/daq_config_server/app/_file_converter_map.py

src/daq_config_server/models/lookup_tables/insertion_device/__init__.py

oliwenmandiamond

I haven't updated any logic or tests, only structure (except when the tests where failing due to state persisting)

oliwenmandiamond · 2026-03-06T10:36:14Z

src/daq_config_server/app/_convert.py

-from ._converter_utils import ConverterParseError
+import daq_config_server.converters._file_converter_map as file_converter_map
+from daq_config_server.converters._converter_utils import ConverterParseError
+from daq_config_server.models._base_model import ConfigModel


 def get_converted_file_contents(file_path: Path) -> dict[str, Any]:


Putting something in __all__ signals that it is part of the public API and intended for external use. It is and should only be used inside this package I believe so doesn't make sense to.

src/daq_config_server/app/_file_converter_map.py

src/daq_config_server/models/lookup_tables/insertion_device/__init__.py

olliesilvester

Thanks, I'm happy with this. It'll probably become obvious if the structure is sensible as we start using all these bits a bit more

src/daq_config_server/app/client.py

fajinyuan

please see my comments so far.

docs/how-to/config-server-guide.md

src/daq_config_server/app/_file_converter_map.py

src/daq_config_server/app/_log.py

src/daq_config_server/app/_routes.py

src/daq_config_server/app/_log.py

src/daq_config_server/app/_routes.py

tests/system_tests/test_client.py

tests/unit_tests/app/test_client.py

oliwenmandiamond · 2026-03-12T10:36:07Z

@jacob720 you good with me merging this now?

jacob720 · 2026-03-12T10:55:29Z

Yeah looks good!

oliwenmandiamond added 2 commits March 3, 2026 14:47

Reorganise repo structure

2b4f75e

Restructure repo

5b46c23

jacob720 reviewed Mar 3, 2026

View reviewed changes

oliwenmandiamond added 14 commits March 4, 2026 10:13

Fix leaking tests

a3dcd20

Fix lint

5dc8c09

Rename plugin to model

b1eba38

Move ConfigModel to models, moved config to app

9c34014

Move constants to where they are actually used

9f44463

Move app to its own package

afefebf

Update tests to mirror new structure

a1dde95

Update test converter name

4f7cc49

Move app tests to reflect package locations

965c880

Remove duplicate test

020ceda

Restructured models, simplified converters

aebd2fd

Rename LookupTableBase cls method to from_contents

c921288

Separate models and tests

0639c12

Further restucture models

afde9a8

Move converter utils to model utils as this is where it is only used.

c9ebfb4

olliesilvester changed the title ~~Redesign structure to use plugin structure~~ Breaking: Redesign structure to use plugin structure Mar 5, 2026

olliesilvester suggested changes Mar 5, 2026

View reviewed changes

oliwenmandiamond commented Mar 6, 2026

View reviewed changes

Relm-Arrowny mentioned this pull request Mar 6, 2026

Move insertion device lut to daq-config-server #153

Draft

oliwenmandiamond and others added 12 commits March 6, 2026 14:01

Move converters into app. Made everything private but api

3dfe77f

Use abs import

d8d74cb

Make ConfigModel public

78d3fa9

Remaining import fixes

2bb8b71

Fix duplicate function and fix tests

6ce0697

Fix lint?

6d4ef3a

Merge branch 'main' into use_plugin_structure

3f9af7f

Fix lint

62f5f09

Change ConfigServer to ConfigClient

da0c70f

Correct docs

43f4bb1

Undo conf.py changes

2accd93

Remove comments

c0e7b0c

olliesilvester approved these changes Mar 10, 2026

View reviewed changes

src/daq_config_server/app/client.py Outdated Show resolved Hide resolved

Use relative import for private modules

f4d1f47

fajinyuan reviewed Mar 10, 2026

View reviewed changes

Update docs to reflect config_client

bf1d75e

fajinyuan reviewed Mar 10, 2026

View reviewed changes

oliwenmandiamond added 4 commits March 10, 2026 15:08

Correct tests to use client rather than server

5d0433a

Remove test class

530e7ef

Update test names

2873893

Update system test

2b7bc4c

fajinyuan approved these changes Mar 10, 2026

View reviewed changes

jacob720 approved these changes Mar 12, 2026

View reviewed changes

oliwenmandiamond merged commit 6df7251 into main Mar 12, 2026
15 checks passed

oliwenmandiamond deleted the use_plugin_structure branch March 12, 2026 10:58

This was referenced Mar 12, 2026

Reflect new config server structure DiamondLightSource/dodal#1977

Merged

Remove mx config client DiamondLightSource/mx-bluesky#1673

Merged

Conversation

oliwenmandiamond commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacob720 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob720 Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

olliesilvester commented Mar 3, 2026

Uh oh!

oliwenmandiamond commented Mar 3, 2026

Uh oh!

jacob720 commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliwenmandiamond commented Mar 4, 2026

Uh oh!

olliesilvester commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliwenmandiamond commented Mar 4, 2026

Uh oh!

oliwenmandiamond commented Mar 5, 2026

Uh oh!

oliwenmandiamond commented Mar 5, 2026

Uh oh!

olliesilvester left a comment

Choose a reason for hiding this comment

Uh oh!

olliesilvester Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

oliwenmandiamond Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

olliesilvester Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olliesilvester Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oliwenmandiamond left a comment

Choose a reason for hiding this comment

Uh oh!

oliwenmandiamond Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

olliesilvester left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fajinyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oliwenmandiamond commented Mar 12, 2026

Uh oh!

jacob720 commented Mar 12, 2026

oliwenmandiamond commented Mar 3, 2026 •

edited

Loading

jacob720 left a comment •

edited

Loading

jacob720 commented Mar 3, 2026 •

edited

Loading

olliesilvester commented Mar 4, 2026 •

edited

Loading

olliesilvester Mar 6, 2026 •

edited

Loading