Make name separator configurable

[coretl]

Remove the stripping of underscore while we're at it, this can be done by overriding set_name if required.

Fixes #666

[tacaswell]

• positional only for the new kwarg?
• should it be threaded through __init__ in at least some of the examples?

[Tom-Willemsen]

This is fine for us at ISIS, and corresponds with my understanding of the dev meeting yesterday. If I was going to use a custom separator it would be useful to have it mentioned briefly in the docs somewhere.

[DominicOram]

Generally looks good but I think we need something in the docs about the tradeoffs of setting it and the reasoning behind making it different by default

[coretl]

• should it be threaded through __init__ in at least some of the examples?

Thinking a bit more about this, I'm wondering if we should remove the ability to set name from __init__ in the first place. It's annoying having to thread it through everywhere, and we generally call set_name on the device after it is created anyway. To ease the usage we could return self from set_name, possibly renaming it to named while we're at it, so instead of:

device = MyDevice("PV_PRE", name="foo")

we would do

device = MyDevice("PV_PRE").named("foo")

This would make it obvious where to put the separator if we wanted to override it without having to thread it through init:

device = MyDevice("PV_PRE").named("foo", separator="_")

• positional only for the new kwarg?

I'm not convinced, I think the name adds something, maybe it should be child_name_separator like you've done in bluesky/ophyd#1222 though?

@tacaswell @DominicOram @Tom-Willemsen thoughts?

[Tom-Willemsen]

we would do

device = MyDevice("PV_PRE").named("foo")

That's a bigger change than originally proposed - that'll need every ophyd-async device and corresponding instantion functions updating, right? At ISIS that's not a huge deal, we've only deployed to one beamline and a handful of test machines so far, but I can imagine it's going to be a bigger churn for dodal etc at Diamond?

---

Under that model our instantiation wrappers for lots of devices at ISIS would basically look like:

# When bluesky name is related to PV name (which is common for us at ISIS)
def my_device_1(some, args, name):
    return MyDevice1(some, args, name).named(name)

# Or for when we want to have a default name for the device,
# only overridden in special cases (99% of the time "THING" is correct).
def my_device_2(some, args, name="THING"):
    return MyDevice2(some, args).named(name)

I don't necessarily mind doing that, the above is quite simple if a bit repetitive, but especially in the second case it's an extra layer of indirection that we need to set up as a consumer of ophyd-async, and an extra thing downstream consumers need to remember to do if they ever want to create MyDevice2 directly.

---

We could live with either case, but I'd say I slightly prefer the existing approach.

[tacaswell]

I like keyword only as comparing

obj.set_name('foo', '-')
obj.set_name('foo', separator='-')
obj.set_name('foo', child_name_separator='-')

the first is pretty unclear. I like the longer name, but at NSLS-II we have the rule "Don't let Tom name things".

---

There is a gentle path to adding obj.named() as you don't have to on day 1 make Obj(..., name='bob') fail as the two methods (set in __init__ and "fix later") are already co-existing.

[coretl]

That's a bigger change than originally proposed - that'll need every ophyd-async device and corresponding instantion functions updating, right? At ISIS that's not a huge deal, we've only deployed to one beamline and a handful of test machines so far, but I can imagine it's going to be a bigger churn for dodal etc at Diamond?

True, but we are about to move to a model where the name of the instantiation function is also used for the returned device, so shouldn't be used in more than a handful of cases...

There is a gentle path to adding obj.named() as you don't have to on day 1 make Obj(..., name='bob') fail as the two methods (set in __init__ and "fix later") are already co-existing.

Ok, I hear the message about a gentle path, but I would also like to get 1.0 out soon and passing name in init is a making it a little messy. There is also DeviceCollector which names things and will continue to work without change.

I suggest:

• Add Device.named(name: str, *, child_name_separator="-") -> Self
• Make Device.set_name a deprecated wrapper to the above that emits a warning
• Make Device.__init__(name="something-non-blank") a deprecated argument that emits a warning
• Remove set_name and the init arg for 1.0

What do you reckon?

[coretl]

@DiamondJoseph what's happening with the device factory wrapper in dodal? Am I right in thinking that this wouldn't cause us too much pain?

[DiamondJoseph]

what's happening with the device factory wrapper in dodal? Am I right in thinking that this wouldn't cause us too much pain?

The device_factory decorator is back in, and can name the device if it has not been named prior to this when called - which is almost certainly done prior to connect? We can modify this call however we want to.

It's only in use on i22, training_rigs, with PRs for non-MX beamlines open while I track down the state of those beamlines.

[coretl]

Ok, a slight modification to the suggestion:

• Keep Device.set_name(name: str, *, child_name_separator="-") -> None
• Add a utility function ophyd_async.core.named_device(device: T, name: str, *, child_name_separator="-") -> T
• Make Device.__init__(name="something-non-blank") a deprecated argument that emits a warning
• Remove the init arg for 1.0

Then we can still write our one-liner if we need to:

device = named_device(MyDevice("PV_PRE"), "foo", child_name_separator="_")

[whs92]

@coretl does the utility function allow us to set the child separator for all ophyd-async devices in a particular environment?

Or is the intention we have to do it per instance or per class definition?

Sorry I've not had enough time to follow this discussion in depth after the call in Monday.

[coretl]

Or is the intention we have to do it per instance or per class definition?

Per instance

does the utility function allow us to set the child separator for all ophyd-async devices in a particular environment?

No, but we can make DeviceCollector do that:

with DeviceCollector(set_name=True, child_name_separator="_"):
    foo = MyDevice("PREFIX1")
    bar = MyDevice("PREFIX2")
assert foo.child.name == "foo_child"
assert bar.another_child.name == "bar_another_child"

[whs92]

No, but we can make DeviceCollector do that

Then for me, this works

[coretl]

I baulked at the sheer number of tests that would have to change if we deprecated passing name to Device.__init__, so you'll be pleased to hear this is now back to the original scope of the PR

[DominicOram]

Minor comment, take it or leave it, otherwise all good from my perspective.