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

ODC plugin produces errors for statically configured channels #19

Open
rbx opened this issue Jun 15, 2021 · 9 comments
Open

ODC plugin produces errors for statically configured channels #19

rbx opened this issue Jun 15, 2021 · 9 comments

Comments

@rbx
Copy link
Member

rbx commented Jun 15, 2021

Describe the bug
Our DDS plugin unconditionally writes DDS properties for each binding channel with the address as the value. However, this is not needed when the channel is configured statically. In this case it will produce an error when trying to write this property, when no properties are defined in the topology definition:

[10:18:59][ERROR] DDS Error received: error code: 3, error message: Can't propagate property <fmqchan_data1> that doesn't exist for task <Sampler>

The topology still works after this error if the static channel configuration is sufficient, so it can be ignored.

To Reproduce
Modify ex-dds-topology.xml from the DDS example as follows:

    <decltask name="Sampler">
-        <exe>fairmq-ex-dds-sampler --color false --channel-config name=data1,type=push,method=bind -P dds --iterations 10</exe>
+        <exe>fairmq-ex-dds-sampler --color false --channel-config name=data1,type=push,method=bind,address=ipc://a -P dds --iterations 10</exe>
        <env reachable="false">fairmq-ex-dds-env.sh</env>
        <requirements>
            <name>SamplerWorker</name>
        </requirements>
-        <properties>
-            <name access="write">fmqchan_data1</name>
-        </properties>
    </decltask>

    <decltask name="Processor">
-        <exe>fairmq-ex-dds-processor --color false --channel-config name=data1,type=pull,method=connect name=data2,type=push,method=connect -P dds</exe>
+        <exe>fairmq-ex-dds-processor --color false --channel-config name=data1,type=pull,method=connect,address=ipc://a name=data2,type=push,method=connect -P dds</exe>
        <env reachable="false">fairmq-ex-dds-env.sh</env>
        <requirements>
            <name>ProcessorWorker</name>
        </requirements>
        <properties>
-            <name access="read">fmqchan_data1</name>
            <name access="read">fmqchan_data2</name>
        </properties>
    </decltask>

Expected behavior
When static config is present, no error should be produced here.

Possible solutions

  • Prior to writing the property, we can check if it is defined. And if it is not, just write debug log.
  • Check if the connecting channel is configured (has a valid address set) in the topology and not write anything if it is.
  • Some combination of the above.
@rbx rbx self-assigned this Jun 15, 2021
@rbx
Copy link
Member Author

rbx commented Jun 15, 2021

One way of checking if a property is defined, could be via the Topology API:

dds::topology_api::CTopology topo("bla.xml");
topo.getRuntimeTaskById(1234).m_task->getProperty("propName");

which would return nullptr for undefined props.

However, this would mean the topo file has to be accessible for the tasks?

It would be nice if the functionality of checking property existence would be available via dds::intercom_api::CKeyValue, which we use to write the property. What do you think @AndreyLebedev ?

@AndreyLebedev
Copy link
Collaborator

AndreyLebedev commented Jun 15, 2021

However, this would mean the topo file has to be accessible for the tasks?

Technically it's possible. The topology file is copied to the working directory and dds-agent is aware of the current topology. I see a potential issue if the topology is updated. The topology user also needs to track those updates.

It would be nice if the functionality of checking property existence would be available via dds::intercom_api::CKeyValue, which we use to write the property. What do you think @AndreyLebedev ?

We need to think about the simplified API for this case. Not sure if it has to be part of dds::intercom_api::CKeyValue or CTopology class.

@dennisklein
Copy link
Member

It would be nice if the functionality of checking property existence would be available via dds::intercom_api::CKeyValue, which we use to write the property. What do you think @AndreyLebedev ?

We need to think about the simplified API for this case. Not sure if it has to be part of dds::intercom_api::CKeyValue or CTopology class.

In this particular case, for a start, I think we should to just update our dds error callback and ignore this error (or demote it to trace/debug).

For a more sophisticated and more general solution, a CTopology getTopology() type of function that can be called from the task would be interesting (+ some way to subscribe on updates to avoid the need to poll). Or at least a CTopoTask getCurrentTask()? One could also create a special Topology/Task type just for usage inside a task and all accessors are implemented with a lock and then all the update logic could be hidden even. I have no particular preference, just throwing in what comes to mind.

@dennisklein dennisklein transferred this issue from FairRootGroup/FairMQ Jul 14, 2021
@dennisklein dennisklein changed the title DDS plugins produces errors for statically configured channels ODC plugin produces errors for statically configured channels Jul 14, 2021
@davidrohr
Copy link
Contributor

I just came across the same problem. These errors are producing an error message flood in global commissioning tests. Would be good if we could fix that with the next version.

@davidrohr
Copy link
Contributor

Is this workaround already in an ODC release?
Our logs are still flooded by messages like

DPL: DDS Error received: error code: 3, error message: Can't propagate property <fmqchan_from_tpc-raw-to-digits-0_t2_to_tpc-zsEncoder_t2> that doesn't exist for task <tpc-raw-to-digits-0_t2>

@rbx
Copy link
Member Author

rbx commented Sep 21, 2021

Yes, but to use it you need to use the ODC plugin and not DDS plugin (otherwise they are identical atm).
Find more details here: FairRootGroup/FairMQ#392

@davidrohr
Copy link
Contributor

You mean when we export the DDS topology, we have to use --plugin odc instead of --plugin dds?
@ktf Can you change this, after @rbx confirms?

@rbx
Copy link
Member Author

rbx commented Sep 21, 2021

Yes, that's correct.

@ktf
Copy link

ktf commented Sep 21, 2021 via email

@rbx rbx removed their assignment May 13, 2022
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

No branches or pull requests

5 participants