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

refactor code to accomodate Nuitka compilation to C++ code #3154

Conversation

sorinansys
Copy link

Description

this is the proposed solution for Issue 3153

The inlined Python code used to retrieve the _RUNTIME_CONTEXT , although non-ambiguous in Python, is not clear enough
for Nuitka compiler to generate C++ functionality with the same functionality as the one in original Python code

Nuitka generates C++ code that compiles and link successfully, but it generates error at runtime.

Fixes # (issue)

The code to retrieve _RUNTIME_CONTEXT is refactored without alteration of functionality, using a slightly explicit coding style that accommodates the Nuitka compiler and facilitates generation of C++ code with the same functionality as the original OpenTelemetry code

Type of change

  • New feature (non-breaking change which adds functionality)
    Python functionality is unchanged.
    The new feature is that OpenTelemetry code can be compiled with Nuitka without this runtime error

How Has This Been Tested?

Tests were performed using the test case in Issue 3153

test results before the change:

(dev_venv) D:\opentelemetry_nuitka_issue>test_case.exe
Logging foo from test_case.exe
Failed to load context: contextvars_context
Traceback (most recent call last):
  File "C:\Users\smuntean\AppData\Local\Temp\ONE57C~1\test_case.py", line 18, in <module>
    log.info(f"Logging foo from {sys.argv[0]} ")
  File "C:\Users\xxxxxxxxx\AppData\Local\Temp\ONE57C~1\logging\__init__.py", line 1477, in info
  File "C:\Users\xxxxxxxxx\AppData\Local\Temp\ONE57C~1\logging\__init__.py", line 1624, in _log
  File "C:\Users\xxxxxxxxx\AppData\Local\Temp\ONE57C~1\logging\__init__.py", line 1634, in handle
  File "C:\Users\xxxxxxxxx\AppData\Local\Temp\ONE57C~1\logging\__init__.py", line 1696, in callHandlers
  File "C:\Users\xxxxxxxxx\AppData\Local\Temp\ONE57C~1\logging\__init__.py", line 968, in handle
  File "C:\Users\xxxxxxxxx\AppData\Local\Temp\ONE57C~1\opentelemetry\sdk\_logs\_internal\__init__.py", line 382, in emit
  File "C:\Users\xxxxxxxxx\AppData\Local\Temp\ONE57C~1\opentelemetry\sdk\_logs\_internal\__init__.py", line 361, in _translate
  File "C:\Users\xxxxxxxxx\AppData\Local\Temp\ONE57C~1\opentelemetry\trace\propagation\__init__.py", line 48, in get_current_span
  File "C:\Users\xxxxxxxxx\AppData\Local\Temp\ONE57C~1\opentelemetry\context\__init__.py", line 96, in get_value
  File "C:\Users\xxxxxxxxx\AppData\Local\Temp\ONE57C~1\opentelemetry\context\__init__.py", line 67, in wrapper
  File "C:\Users\xxxxxxxxx\AppData\Local\Temp\ONE57C~1\opentelemetry\context\__init__.py", line 131, in get_current
AttributeError: 'NoneType' object has no attribute 'get_current'

(dev_venv) D:\opentelemetry_nuitka_issue>

Test result after change:
test results before the change:

(dev_venv) D:\opentelemetry_nuitka_issue>test_case.exe
Logging foo from test_case.exe
(dev_venv) D:\opentelemetry_nuitka_issue>

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8
  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md
  • Yes. - Link to PR:

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@sorinansys sorinansys requested a review from a team February 1, 2023 22:38
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 1, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@aabmass
Copy link
Member

aabmass commented Feb 10, 2023

@sorinansys can you sign the CLA #3154 (comment)?

@aabmass
Copy link
Member

aabmass commented Feb 10, 2023

Can you see if the issue is still happening on the main branch since #3047 was merged? We are no longer using pkg_resources here.

@sorinansys
Copy link
Author

@sorinansys can you sign the CLA #3154 (comment)?

@aabmass , I am currently working with my employer to get the CLA manager signature. Should not take long. Thank you for the reminder.

@sorinansys
Copy link
Author

Can you see if the issue is still happening on the main branch since #3047 was merged? We are no longer using pkg_resources here.

Sure. I will checkout the code, run the tests and let you know the outcome.

@sorinansys
Copy link
Author

Can you see if the issue is still happening on the main branch since #3047 was merged? We are no longer using pkg_resources here.

Hi @aabmass I confirm that I can still see the issue after the #3407 was merged in main. I will merge main on my branch and then accommodate the merged code to Nuitka compilation. Sorry for the late response. PR to be updated soon.

@sorinansys sorinansys closed this Feb 27, 2023
@sorinansys sorinansys force-pushed the sorin-ansys-nuitka-friendly-patch branch from 1b1daa7 to a9a96aa Compare February 27, 2023 19:37
@sorinansys sorinansys reopened this Feb 27, 2023
Copy link
Author

@sorinansys sorinansys left a comment

Choose a reason for hiding this comment

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

With these changes applied, Nuitka compilation generates correct C++ code that mimics the functionality in the Python code

@ocelotl
Copy link
Contributor

ocelotl commented Mar 14, 2023

@sorinansys can you try something else? This may be related to an issue we have with the entry_points function not having the same API for every Python version. Please try with the code in #3168 and let us know how that goes.

@sorinansys
Copy link
Author

@sorinansys can you try something else? This may be related to an issue we have with the entry_points function not having the same API for every Python version. Please try with the code in #3168 and let us know how that goes.

@ocelotl The code in #3168 does not resolve the issue in #3153. Nuitka miscompiles the combination of Python next / iter to C++ code, causing a runtime error. In this PR I refactored the code using next / iter with a for / break to resolve it. Python code is equivalent, but when using for / break Nuitka also generates C++ equivalent code.

Please not that when running test case in #3153 with the most recent opentelemetry code an additional Nuitka runtime error is thrown when importing opentelemetry.sdk.resources module. I am looking to see if there is a solution for that error also

Thank you

1 similar comment
@sorinansys
Copy link
Author

@sorinansys can you try something else? This may be related to an issue we have with the entry_points function not having the same API for every Python version. Please try with the code in #3168 and let us know how that goes.

@ocelotl The code in #3168 does not resolve the issue in #3153. Nuitka miscompiles the combination of Python next / iter to C++ code, causing a runtime error. In this PR I refactored the code using next / iter with a for / break to resolve it. Python code is equivalent, but when using for / break Nuitka also generates C++ equivalent code.

Please not that when running test case in #3153 with the most recent opentelemetry code an additional Nuitka runtime error is thrown when importing opentelemetry.sdk.resources module. I am looking to see if there is a solution for that error also

Thank you

@sorinansys
Copy link
Author

@aabmass @ocelotl , I've run several tests with a newer Nuitka (version 1.5.3) and the compilation of next(iter) Python code seems to generate C++ code with equivalent functionality. With the newer version of Nuitka the for/break refactory of next(iter) Python code does not seem to be needed.

Issue #3153 is specific to Nuitka version 1.4.1. One can close the issue as described with the suggestion to update Nuitka, or merge this PR to make OTEL code compatible with older Nuitka version. I found both options reasonable. I will let you decide how to proceed.

I will also add this comment in Issue #3153

@srikanthccv
Copy link
Member

@sorinansys I will close this since you mentioned it works with 1.5.3. Also, I want to point out that we officially support CPython and PyPy (with some exceptions). Anything outside that, such as Nutika, it is their responsibility to keep the compatibility with CPython. Thank you.

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