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

Update v6.0.0.1 #37

Merged
merged 32 commits into from
Mar 7, 2024
Merged

Update v6.0.0.1 #37

merged 32 commits into from
Mar 7, 2024

Conversation

SerodioJ
Copy link
Contributor

@SerodioJ SerodioJ commented Mar 6, 2023

  • Updates lib to use PAPI v6.0.0.1
  • Implements bindings for almost all low level APIs
  • Adds functionalities to convert from C structs to Python objects
  • Adds Dockerfile to build image

@flozz
Copy link
Owner

flozz commented Mar 6, 2023

Hello,

First, thank you for your work!

I had a quick look of the PR, here are two things I noticed:

  • The documentation (docs/ folder) has no changes but may need to be updated for the new APIs
  • Please do not bump the version number in the setup.py file, this should be done at release time :)

I will try to do a complete review by the end of the week. Once merged, I will add some lint checking and an automated build for sdist and wheel package with automatic publishing on PyPI :)

@SerodioJ
Copy link
Contributor Author

SerodioJ commented Mar 6, 2023

Hi flozz,

Thank you for your quick feedback.
I will update the documentation to reflect the new APIs and also revert the version number in setup.py as you mentioned.

@flozz
Copy link
Owner

flozz commented Mar 12, 2023

Hello,

I reviewed the PR, I only have a remark and a question:

  • The documentation is not up to date with the new code and should be updated before we can merge the PR
  • Why using numpy types instead of native Python ones?

Apart from these two points, everything is looking good to me :)

@mknull
Copy link

mknull commented Sep 22, 2023

This looks very useful. Are there any plans to add this to master?

@flozz
Copy link
Owner

flozz commented Sep 22, 2023

This looks very useful. Are there any plans to add this to master?

It is not merged because the doc is not updated and does not match the new APIs. As soon as the doc is up to date with the code, it could be merged... (also I do not know if the numpy arrays are really useful here...)

@SerodioJ
Copy link
Contributor Author

Hi, everyone.

I had a kinda of chaotic semester, so I ended up forgetting about this PR.
I will try to update the documentation to reflect the new APIs in the following couple of weeks.

Regarding the usage of NumPy, I ended up using it to enforce C types on integer values as Python's int has variable size, so it could lead to some problems when converting data to/from PAPI.
If any of you have a better solution I could try to refactor this as well.

Sorry for the late reply.

@flozz
Copy link
Owner

flozz commented Sep 25, 2023

If any of you have a better solution I could try to refactor this as well.

Maybe the ctypes module could help. But if you think NumPy is the best solution, it is ok for me (I just wanted to know why it was added :) )

Sorry for the late reply.

Do not worry! We all have our own problems and priorities, so update the doc when you have time for it ;)


# Stop counters
papi_high.stop_counters() # -> []
PAPI_hl_region_end("computation")
Copy link

Choose a reason for hiding this comment

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

Suggested change
PAPI_hl_region_end("computation")
papi_high.hl_region_end("computation")

I assume


rcode = lib.PAPI_start_counters(events_, array_len)
cregion = ffi.new("char[]", region.encode("ascii"))
rcode = lib.PAPI_hl_region(cregion)
Copy link

Choose a reason for hiding this comment

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

Getting AttributeError: cffi library 'pypapi._papi' has no function, constant or global variable named 'PAPI_hl_region'. Did you mean: 'PAPI_hl_read'? when trying to call this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WaDelma thanks for pointing out these problems.
I fixed them on the commits below.
Could you test the lib?

I still have to validate if the changes from NumPy to CTypes that I made did not mess any masking operations.

@SerodioJ
Copy link
Contributor Author

Hi everyone.
I finally found the time to update the docs and change from NumPy to Ctypes.
I will just check some things on the low level API and I believe it will be ready to merge.

@WaDelma
Copy link

WaDelma commented Feb 25, 2024

I am trying to do:

os.environ["PAPI_EVENTS"] = "PAPI_TOT_CYC"
os.environ["PAPI_OUTPUT_DIRECTORY"] = "."
papi_high.hl_region_begin("computation")
function()
papi_high.hl_region_end("computation")

and while it doesn't error, it also doesn't seem to create any file anywhere. I found information that it should create folder papi_hl_output, but searching my whole computer none named as such exist.

I am quite new to PAPI, so might have just missed something.

@SerodioJ
Copy link
Contributor Author

SerodioJ commented Feb 26, 2024

I am trying to do:

os.environ["PAPI_EVENTS"] = "PAPI_TOT_CYC"
os.environ["PAPI_OUTPUT_DIRECTORY"] = "."
papi_high.hl_region_begin("computation")
function()
papi_high.hl_region_end("computation")

and while it doesn't error, it also doesn't seem to create any file anywhere. I found information that it should create folder papi_hl_output, but searching my whole computer none named as such exist.

I am quite new to PAPI, so might have just missed something.

Could you check the available components of your PAPI installation?
If you installed the library following the steps on the README, at the root of the repository run:

./papi/src/utils/papi_component_avail

Check if the perf_event component is active and has a set of available counters.

If it shows something like this:

Compiled-in components:
Name: perf_event Linux perf_event CPU counters
-> Disabled: Unknown libpfm4 related error
Name: perf_event_uncore Linux perf_event CPU uncore and northbridge
-> Disabled: No uncore PMUs or events found

It's a problem with the version of libpfm4 ported with PAPI.
I believe that updating PAPI to a newer version might solve this issue.
You can test it by going to the submodule folder and checking out to PAPI 7.0.1 version, and installing the Python lib again. There might be some inconsistencies between the Python binding and the newer API, but for the High Level API, it seems to have none.

@flozz
Copy link
Owner

flozz commented Feb 26, 2024

@SerodioJ Is the PR ready for review? If yes, I will try to get it done by the end of the week :)

@SerodioJ
Copy link
Contributor Author

@flozz It is ready. Thanks!!

I will try to update the lib for the current version of PAPI in the near future. It seems that a few functionalities are not working well on newer processors with the version 6.0.0.1 but they work with 7.0.1.

@WaDelma
Copy link

WaDelma commented Feb 26, 2024

Could you check the available components of your PAPI installation? If you installed the library following the steps on the README, at the root of the repository run:

./papi/src/utils/papi_component_avail

Check if the perf_event component is active and has a set of available counters.

If it shows something like this:

Compiled-in components:
Name: perf_event Linux perf_event CPU counters
-> Disabled: Unknown libpfm4 related error
Name: perf_event_uncore Linux perf_event CPU uncore and northbridge
-> Disabled: No uncore PMUs or events found

I got

Compiled-in components:                                                                                  
Name:   perf_event              Linux perf_event CPU counters           
Name:   perf_event_uncore       Linux perf_event CPU uncore and northbridge
   \-> Disabled: Insufficient permissions for uncore access.  Set /proc/sys/kernel/perf_event_paranoid to 0 or run as root.

and papi_avail shows PAPI_TOT_CYC 0x8000003b Yes No Total cycles so should have that counter available.

What I did notice that I actually have PAPI version : 6.0.0.0 which I installed with apt.

@SerodioJ
Copy link
Contributor Author

@WaDelma I created an issue #49 regarding this behavior of not producing anything. Could we move the discussion to that thread?

@flozz flozz mentioned this pull request Mar 5, 2024
@flozz
Copy link
Owner

flozz commented Mar 5, 2024

I reviewed the code and fixed some things. I made a PR on your repo: SerodioJ#1

once this PR merged, I will be able to merge this one and to release a new version :)

@flozz flozz merged commit a096fd4 into flozz:master Mar 7, 2024
5 checks passed
@flozz
Copy link
Owner

flozz commented Mar 7, 2024

→ v6.0.0.1 finally released. wheels are being build and will be published on PyPI soon.

Thank you for the PR! :)

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

Successfully merging this pull request may close these issues.

4 participants