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

Python bindings are non-functional #296

Closed
olepbr opened this issue Apr 2, 2023 · 3 comments
Closed

Python bindings are non-functional #296

olepbr opened this issue Apr 2, 2023 · 3 comments
Projects

Comments

@olepbr
Copy link

olepbr commented Apr 2, 2023

The Python bindings, merged in #184, are non-functional. Some immediate issues:

  • get_energy_consumption_measures() does not actually return any consumption measures; it returns an empty list. This is because it just calls generate_topology() and iterates over the generated Topology's record_buffer, which is empty as the topology was just generated (Topology::new() does record_buffer =vec![]).
  • The PR claimed to add Python documentation, but it merely added an empty Sphinx harness.
  • The supplied Makefile does not work as intended: make install fails due to $(eval TARGET_WHEEL := $(shell ls ../target/wheels/scaphandre-${PACKAGE_VERSION}-*.whl)) looking for the wheel in the parent directory's target/ instead of its own target/, where maturin build was just run.
  • Even the string representation of the class is broken: from scaphandre import Scaphandre; s = Scaphandre(); print(s); results in AttributeError, because the dataclass (which includes all fields in the generated __repr__() unless one specifies repr=False) is defined with sensor_name: str whilst the __init__() sets self.name.
  • Furthermore, the bindings deviate from Scaphandre itself: why is the sensor called _scaphandre instead of sensor, for instance? Given that these are "official" bindings, I think they should have a structure similar to the Rust implementation, or at least be more clearly defined and documented.

I guess all of this ties in with #50; since it's not clear how to use Scaphandre as a Rust crate, making bindings to another language is potentially even less clear. Most of these issues are easily fixable, though, but I do question why these bindings were merged in the first place.

Anyway, I hope this is of help. :-)

@bpetit bpetit added this to Triage in General Apr 11, 2023
@TheElectronWill
Copy link
Contributor

Yes, I think that they should be removed until #50 is completely solved.

@bpetit
Copy link
Contributor

bpetit commented May 22, 2023

Hi !

@fvaleye any clue on a fix ?

I'll merge @TheElectronWill's PR/removal for 1.0 if not fixed before.

Thanks.

@bpetit bpetit closed this as completed Aug 14, 2023
General automation moved this from Triage to Done Aug 14, 2023
@bpetit
Copy link
Contributor

bpetit commented Aug 14, 2023

Removed until a new PR that fixes it :)

@bpetit bpetit moved this from Done to Previous releases in General Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
General
Previous releases
Development

Successfully merging a pull request may close this issue.

3 participants