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.

Sign up for GitHub

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

C handle init #76

Merged
merged 15 commits into from
Mar 12, 2024
Merged

C handle init #76

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
679bd66
add names to some of the core tests in case the brokers haven't been …
phlptp Dec 14, 2021
66a258f
fix test3
phlptp Dec 14, 2021
d6847b4
Merge pull request #41 from GMLC-TDC/update_test_core_names
kdheepak Dec 16, 2021
23a6d2c
Merge branch 'main' of github.com:GMLC-TDC/pyhelics into develop
afisher1 Nov 1, 2022
9de0708
Correct copy-paste error for helicsTranslatorGetName
trevorhardy Aug 24, 2023
a2bc598
Merge pull request #68 from GMLC-TDC/translatorName
trevorhardy Aug 24, 2023
6c95a5c
Add super __init__() for select classes
trevorhardy Feb 2, 2024
4172f01
Add missing ")"
trevorhardy Feb 2, 2024
bb8f737
Add additional fix for `helicsQueryBufferFill()`
trevorhardy Feb 2, 2024
14c1e77
Test change to Python3 version of `super()`
trevorhardy Feb 2, 2024
8c0e639
Fix copy-paste error for inits
trevorhardy Feb 2, 2024
5fa9f76
Fix another copy-paste error
trevorhardy Feb 2, 2024
7f30f42
Update helics/capi.py to use Py v3 super()
trevorhardy Feb 9, 2024
ce8cf15
Update helics/capi.py to use Py v3 super()
trevorhardy Feb 9, 2024
3f77ed4
Update helics/capi.py to use Py v3 super()
trevorhardy Feb 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions helics/capi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1392,11 +1392,13 @@ def message_id(self, v):


class HelicsQuery(_HelicsCHandle):
pass
def __init__(self, handle, cleanup=False):
super(HelicsQuery, self).__init__(handle, cleanup)
Copy link
Contributor

Choose a reason for hiding this comment

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

Having pass should be equivalent to these lines, so I'm not quite sure why this change is needed.

Copy link
Contributor Author

@trevorhardy trevorhardy Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When we just had pass in there we were getting the error below.

    f(buffer.handle, string, len(string), err)
      ^^^^^^^^^^^^^
AttributeError: cdata 'void *' has no attribute 'handle'

Calling the super's init() solved the problem. Generally, I didn't think a class calls it's super's init but you know WAY MORE about these things than I do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're interested @kdheepak , I'm sure we can set up a call to go over this. Or, you can try running the connector example with and without the above changes to capi.py to see if you get similar behavior. You'll need to build the HELICS "dev" branch to get the connector app.

Copy link
Member

Choose a reason for hiding this comment

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

HELICS 3.5 was released, so there should be a pre-built copy of the connector app available now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does Github update automatically or manually? Right now it's showing v3.4.0 as the latest release.
Screenshot 2024-02-09 at 9 59 01 AM

Copy link
Member

Choose a reason for hiding this comment

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

apparently have to actively select a check box for that now, now 3.5 should be the latest

Copy link
Member

Choose a reason for hiding this comment

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

I'm still seeing the checkbox selected by default when I go to draft a release. It might remember the last selection that was made for a release.

I figured it just hadn't been marked as latest while the remaining interfaces were being updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

This error is telling me that buffer is a cdata 'void *' object and has no attribute 'handle'.

    f(buffer.handle, string, len(string), err)
      ^^^^^^^^^^^^^
AttributeError: cdata 'void *' has no attribute 'handle'

That's being called in:

pyhelics/helics/capi.py

Lines 9595 to 9609 in 5516142

def helicsQueryBufferFill(buffer: HelicsQueryBuffer, string: str):
"""
Set the data for a query callback.
There are many queries that HELICS understands directly, but it is occasionally useful to have a federate be able to respond to specific queries with answers specific to a federate.
- **`buffer`**: The buffer received in a helicsQueryCallback.
- **`string`**: Pointer to the data to fill the buffer with.
"""
f = loadSym("helicsQueryBufferFill")
err = helicsErrorInitialize()
f(buffer.handle, string, len(string), err)
if err.error_code != 0:
raise HelicsException("[" + str(err.error_code) + "] " + ffi.string(err.message).decode())

You can see that buffer should be a HelicsQueryBuffer and not a cdata 'void *'. That's telling me that the wrong data is being passed in for some reason.

I do see the HelicsQueryBuffer is being created correctly here:

https://github.com/GMLC-TDC/HELICS-Examples/blob/main/user_guide_examples/advanced/advanced_connector/interface_creation/Charger.py#L169
https://github.com/GMLC-TDC/HELICS-Examples/blob/main/user_guide_examples/advanced/advanced_connector/interface_creation/Battery.py#L116

I'll try to run the example to see what is going on when I get the chance.

trevorhardy marked this conversation as resolved.
Show resolved Hide resolved


class HelicsQueryBuffer(_HelicsCHandle):
pass
def __init__(self, handle, cleanup=False):
super(HelicsQueryBuffer, self).__init__(handle, cleanup)
trevorhardy marked this conversation as resolved.
Show resolved Hide resolved


class HelicsEndpoint(_HelicsCHandle):
Expand Down Expand Up @@ -2823,7 +2825,8 @@ class HelicsCallbackFederate(HelicsCombinationFederate):
pass

class HelicsDataBuffer(_HelicsCHandle):
pass
def __init__(self, handle, cleanup=False):
super(HelicsDataBuffer, self).__init__(handle, cleanup)
trevorhardy marked this conversation as resolved.
Show resolved Hide resolved


class HelicsException(Exception):
Expand Down Expand Up @@ -7269,7 +7272,7 @@ def helicsTranslatorGetName(translator: HelicsTranslator) -> str:

**Returns**: A string with the name of the translator.
"""
f = loadSym("helicsPublicationGetName")
f = loadSym("helicsTranslatorGetName")
result = f(translator.handle)
return ffi.string(result).decode()

Expand Down Expand Up @@ -9364,7 +9367,8 @@ def helicsQueryBufferFill(buffer: HelicsQueryBuffer, string: str):
"""
f = loadSym("helicsQueryBufferFill")
err = helicsErrorInitialize()
f(buffer.handle, string, len(string), err)
str_ptr = ffi.new('char[]', string.encode('utf-8'))
Copy link
Member

Choose a reason for hiding this comment

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

Is HELICS expecting a utf-8 encoded string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what Mark Eberlein put in and it works so I guess, "yes"? It might make more sense to choose ascii. Let me know if you want me to change something.

Copy link
Member

@nightlark nightlark Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like the test code in HELICS is just shoving a regular char * into the query buffer, which seems to imply that any encoding is possible? I think this function should have an argument that specifies the encoding to use (or no encoding/raw bytes). @phlptp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phlptp; any input on what you think we should do here? I don't have a philosophical problem with supporting data structures (raw bytes) as query responses if there's a need to do so BUT I suspect restricting this to ASCII or UTF-8 will make it easier to maintain the code and easier for users to understand what they are doing. Off the top of my head, I can't think of a use case where it would be important to support a raw bytes data structure where something like JSON wouldn't also be sufficient.

Copy link
Member

Choose a reason for hiding this comment

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

I think I would be fine restricting it in python. In the lower levels there are a few use cases for queries/commands getting raw binary data that I want to leave open, but that would be really complicated to use in python so don't really think we would need to support it, and if someone wanted and had the ability they could directly access the library calls.

f(buffer.handle, str_ptr, len(string), err)
if err.error_code != 0:
raise HelicsException("[" + str(err.error_code) + "] " + ffi.string(err.message).decode())

Expand Down
2 changes: 1 addition & 1 deletion tests/test_filters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ def test_filter_type_tests_info():
destroyBroker(broker)


def test_filter_types_tests_core_fitler_registration():
def test_filter_types_tests_core_filter_registration():

core1 = h.helicsCreateCore("inproc", "core1", "--autobroker")

Expand Down
28 changes: 14 additions & 14 deletions tests/test_python_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@


def test_python_api0():
broker = h.helicsCreateBroker("zmq", "", "-f 1 --name=mainbroker")
broker = h.helicsCreateBroker("zmq", "", "-f 1 --name=mainbroker0")
fedinfo = h.helicsCreateFederateInfo()
assert "HelicsFederateInfo()" in repr(fedinfo)
fedinfo.core_name = "TestFederate"
fedinfo.core_type = "zmq"
fedinfo.core_init = "-f 1 --broker=mainbroker"
fedinfo.core_init = "-f 1 --broker=mainbroker0 --name=core0"
mFed = h.helicsCreateCombinationFederate("TestFederate", fedinfo)

assert (
Expand Down Expand Up @@ -61,12 +61,12 @@ def test_python_api0():

def test_python_api1():

broker = h.helicsCreateBroker("zmq", "", "-f 1 --name=mainbroker")
broker = h.helicsCreateBroker("zmq", "", "-f 1 --name=mainbroker1")
fedinfo = h.helicsCreateFederateInfo()
assert "HelicsFederateInfo()" in repr(fedinfo)
fedinfo.core_name = "TestFederate"
fedinfo.core_type = "zmq"
fedinfo.core_init = "-f 1 --broker=mainbroker"
fedinfo.core_init = "-f 1 --broker=mainbroker1 --name=core0"
mFed = h.helicsCreateCombinationFederate("TestFederate", fedinfo)

assert (
Expand Down Expand Up @@ -266,7 +266,7 @@ def test_python_api1():

def test_python_api2():

broker = h.helicsCreateBroker("zmq", "broker", "--federates 1 --loglevel=warning")
broker = h.helicsCreateBroker("zmq", "broker", "--federates 1 --loglevel=warning --name=mainbroker2")
assert broker.is_connected()

broker.set_global("hello", "world")
Expand All @@ -280,7 +280,7 @@ def test_python_api2():
assert broker.query("hello", "world") == {"error": {"code": 404, "message": "query not valid"}}

fi = h.helicsCreateFederateInfo()
fi.core_init = "--federates 1"
fi.core_init = "--federates 1 --brokername=mainbroker2 --name=core2"
fi.property[h.HELICS_PROPERTY_INT_LOG_LEVEL] = 2

fed = h.helicsCreateCombinationFederate("test1", fi)
Expand Down Expand Up @@ -436,15 +436,15 @@ def test_python_api2():


def test_python_api3():
core1 = h.helicsCreateCore("inproc", "core1", "--autobroker")
core1 = h.helicsCreateCore("inproc", "core3", "--autobroker")

assert """HelicsCore(identifier = "core1", address = "core1")""" in repr(core1)
assert """HelicsCore(identifier = "core3", address = "core3")""" in repr(core1)

core2 = core1.clone()

assert core1.identifier == "core1"
assert core1.identifier == "core3"

source_filter1 = core1.register_filter(h.HELICS_FILTER_TYPE_DELAY, "core1SourceFilter")
source_filter1 = core1.register_filter(h.HELICS_FILTER_TYPE_DELAY, "core3SourceFilter")

source_filter1.add_source_target("ep1")

Expand Down Expand Up @@ -504,7 +504,7 @@ def test_python_api4():


def test_python_api5():
broker = h.helicsCreateBroker("zmq", "broker", "--federates 1")
broker = h.helicsCreateBroker("zmq", "broker", "--federates 1 --name=mainbroker5")
fi = h.helicsCreateFederateInfo()

fed = h.helicsCreateCombinationFederate("test1", fi)
Expand All @@ -527,7 +527,7 @@ def test_python_api5():


def test_python_api6():
broker = h.helicsCreateBroker("zmq", "broker", "--federates 1")
broker = h.helicsCreateBroker("zmq", "broker", "--federates 1 --name=mainbroker6")
fi = h.helicsCreateFederateInfo()

fed = h.helicsCreateCombinationFederate("test1", fi)
Expand All @@ -549,7 +549,7 @@ def test_python_api6():

@pt.mark.skip(reason="Fails to pass on windows and linux")
def test_python_api7():
broker = h.helicsCreateBroker("zmq", "broker", "--federates 1")
broker = h.helicsCreateBroker("zmq", "broker", "--federates 1 --name=mainbroker7")
fi = h.helicsCreateFederateInfo()

fed = h.helicsCreateCombinationFederate("test1", fi)
Expand Down Expand Up @@ -612,7 +612,7 @@ def test_python_api7():

def test_python_api8():

broker = h.helicsCreateBroker("zmq", "", "-f 1 --name=mainbroker")
broker = h.helicsCreateBroker("zmq", "", "-f 1 --name=mainbroker8")

cfed = h.helicsCreateCombinationFederateFromConfig(os.path.join(CURRENT_DIRECTORY, "combinationfederate.json"))

Expand Down
Loading