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

C handle init #76

Merged
merged 15 commits into from
Mar 12, 2024
Merged

C handle init #76

merged 15 commits into from
Mar 12, 2024

Conversation

trevorhardy
Copy link
Contributor

Add __init__() definitions for all classes that inherit from "_HelicsCHandle()". This fix was necessary for allowing the implementation of a query callback (specifically providing the init to "HelicsQueryBuffer").

phlptp and others added 7 commits December 14, 2021 12:18
…completely cleared by the start of the next test
add names to some of the core tests in case the brokers haven't been …
Correct copy-paste error for helicsTranslatorGetName
Several classes that were inheriting from "_HelicsCHandle" had no proper init and thus were not able to be used. Theses now have the same init as their superclass ("_HelicsCHandle") and will be more useful.
@trevorhardy trevorhardy added the bug Something isn't working label Feb 2, 2024
@phlptp
Copy link
Member

phlptp commented Feb 2, 2024

Do we know how to fix the failures here

@trevorhardy
Copy link
Contributor Author

For all the functions that inherited from "_HelicsCHandle" I implemented the fix that Mark put together (initializing with the super classes init) and I'm 99% sure that this will provide at least a good-enough band-aid to make things not fail like they were for us. Once we have a new PyHELICS expert they should go through capi.py and figure out how to do this better.

#75

@phlptp
Copy link
Member

phlptp commented Feb 2, 2024

Getting a lot of

tests/test_api.py:13: in <module>
    import helics as h
helics/__init__.py:2: in <module>
    from .capi import *
E     File "/Users/runner/work/pyhelics/pyhelics/helics/capi.py", line 1477
E       class HelicsQueryBuffer(_HelicsCHandle):
E       ^
E   SyntaxError: invalid syntax

in the testing

@trevorhardy
Copy link
Contributor Author

Let me see if there's something I've obviously done wrong.

@trevorhardy
Copy link
Contributor Author

Stupid missing ")", double-checking I didn't copy-paste this everywhere and then I'll push back up. Should be just a minute.

Because Trevor can't copy-and-paste reliably.
@trevorhardy
Copy link
Contributor Author

Ok, let's try this again.

Gotta handle strings and pointers properly.
@trevorhardy
Copy link
Contributor Author

I'm feeling good about this one....

@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (2f495af) 64.20% compared to head (3f77ed4) 63.93%.
Report is 29 commits behind head on main.

Files Patch % Lines
helics/capi.py 44.44% 5 Missing ⚠️
tests/test_python_api.py 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
- Coverage   64.20%   63.93%   -0.27%     
==========================================
  Files          27       27              
  Lines        8401     8405       +4     
==========================================
- Hits         5394     5374      -20     
- Misses       3007     3031      +24     
Flag Coverage Δ
unit 63.93% <76.00%> (-0.27%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nightlark
Copy link
Member

Maybe the filter test should be marked as skip on Windows? Access violation sounds like it is crashing in the C/C++ part of the HELICS library.

@trevorhardy
Copy link
Contributor Author

I don't know enough to say whether skipping that test is a good idea but I like seeing one more green checkmark.

@nightlark
Copy link
Member

nightlark commented Feb 3, 2024

It is strange how it passes for the CI job triggered by the "push"/commit, but fails for the "pull_request" trigger; also the test passed in the CI job for the merged PR earlier passed in the main branch.

@@ -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

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.

helics/capi.py Outdated
Comment on lines 1395 to 1396
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

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.

Copy link
Member

@nightlark nightlark left a comment

Choose a reason for hiding this comment

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

Use Python 3 style super().

helics/capi.py Outdated Show resolved Hide resolved
helics/capi.py Outdated Show resolved Hide resolved
helics/capi.py Outdated Show resolved Hide resolved
@trevorhardy
Copy link
Contributor Author

Use Python 3 style super().

If we've dropped Python2 support, I'm on board. If we haven't, we should keep it the same way.

@nightlark
Copy link
Member

nightlark commented Feb 9, 2024

We dropped Python 2 support. Our CI jobs couldn't even install Python 2.7 to run tests without failing.

trevorhardy and others added 3 commits February 9, 2024 11:26
Co-authored-by: Ryan Mast <3969255+nightlark@users.noreply.github.com>
Co-authored-by: Ryan Mast <3969255+nightlark@users.noreply.github.com>
Co-authored-by: Ryan Mast <3969255+nightlark@users.noreply.github.com>
@phlptp phlptp merged commit 1a38094 into main Mar 12, 2024
5 of 6 checks passed
@phlptp phlptp deleted the CHandle_init branch March 12, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants