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

Optional error message conforming to Python return. KXI-1467 #100

Merged
merged 12 commits into from
May 11, 2021

Conversation

cmccarthy1
Copy link
Contributor

@cmccarthy1 cmccarthy1 commented Apr 22, 2021

Previously embedPy only provided the return 'value' of an error retrieved from Python rather than the full traceback. This is shown in the following example
a.py

import numpy as np

class Element:
    def __init__(self): pass

a = Element()
periodicTable = np.array(range(7*32)).reshape((7,32))
periodicTable[0][0] = a

When imported this file causes an error however it does not provide information relating to the location of the error in the loaded file as would be provided by the Python traceback

$ q p.q
q).p.import[`a]
'import: int() argument must be a string, a bytes-like object or a number, not 'Element'

Users can now modify how errors are returned within their system by running .p.setpyerr[1] which will update the format that the error message gets returned, namely returning the error traceback if it can be formed so that it can be parsed by the user within a protected execution. This is left to the user.

$ q p.q
q).p.setpyerr[1]
q).p.getpyerr[]
1
q).p.import[`a]
'import: ['Traceback (most recent call last):\n', '  File "/Users/conormccarthy/projects/fusion/embedpy/embedpy_conor/a.py", line 8, in <module>\n    periodicTable[0][0] = a\n', "TypeError: int() argument must be a string, a bytes-like object or a number, not 'Element'\n"]
q).p.setpyerr[0]
q).p.import[`a]
'import: int() argument must be a string, a bytes-like object or a number, not 'Element'
q).p.getpyerr[]
0
  • Updated the makefile to move p.q and p.k to $QHOME in line with any changes to these files
  • Updated the .gitignore to ignore any artefacts generated during the make process
  • Updated the makefile to make the p.so during a make install if not already generated
  • Updated the buffer logic for error message display, previously the fixed maximum buffer size had no protection against buffer overflow. This was not an issue as the buffer size was much greater than could be expected to be returned from the truncated error messaging. With traceback this is no longer the case.

@cmccarthy1 cmccarthy1 requested a review from awilson-kx April 22, 2021 09:13
Copy link
Contributor

@awilson-kx awilson-kx left a comment

Choose a reason for hiding this comment

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

Looks good to me

@cmccarthy1 cmccarthy1 merged commit 7ccb462 into KxSystems:master May 11, 2021
cmccarthy1 added a commit that referenced this pull request May 18, 2021
* virtual env

* removed setting of python home

* cleaned up p.q

* working version for working environments

* update base prefix

* addition of py_SetProgramName

* added travis virtualenv

* added tests for sys commands. Added warning for windows virtual environment users

* fix windows venv warning

* Fixed stdOut issue (#101)

* Optional error message conforming to Python return. KXI-1467 (#100)

* Update to p.q logic for handling sys.argv

* reintroduction of old logic

* addition of initial sys argv and embedpy version check

* Addition of functionality to return Python traceback rather than q shorthand

* Memory leak fix and removal of environment variable check C side

* reformat of prr to use modifiable global rather than env variable

* Initial pass at rectifying potential buffer overflow

* update to py.c to initialize traceback import on init

* closer fit to code base style

Co-authored-by: Conor McCarthy <conormccarthy@brainpool1.mynet>

Co-authored-by: cmccarthy1 <38653604+cmccarthy1@users.noreply.github.com>
Co-authored-by: Conor McCarthy <conormccarthy@brainpool1.mynet>
cmccarthy1 added a commit that referenced this pull request May 21, 2021
* venv support (#99)

* virtual env

* removed setting of python home

* cleaned up p.q

* working version for working environments

* update base prefix

* addition of py_SetProgramName

* added travis virtualenv

* added tests for sys commands. Added warning for windows virtual environment users

* fix windows venv warning

Co-authored-by: Dianeod <40861871+Dianeod@users.noreply.github.com>
Co-authored-by: Diane ODonoghue <dodonoghue@kx.com>

* Venv (#102)

* virtual env

* removed setting of python home

* cleaned up p.q

* working version for working environments

* update base prefix

* addition of py_SetProgramName

* added travis virtualenv

* added tests for sys commands. Added warning for windows virtual environment users

* fix windows venv warning

* Fixed stdOut issue (#101)

* Optional error message conforming to Python return. KXI-1467 (#100)

* Update to p.q logic for handling sys.argv

* reintroduction of old logic

* addition of initial sys argv and embedpy version check

* Addition of functionality to return Python traceback rather than q shorthand

* Memory leak fix and removal of environment variable check C side

* reformat of prr to use modifiable global rather than env variable

* Initial pass at rectifying potential buffer overflow

* update to py.c to initialize traceback import on init

* closer fit to code base style

Co-authored-by: Conor McCarthy <conormccarthy@brainpool1.mynet>

Co-authored-by: cmccarthy1 <38653604+cmccarthy1@users.noreply.github.com>
Co-authored-by: Conor McCarthy <conormccarthy@brainpool1.mynet>

* Explicit note that venv support limitation is only expected to hold for Windows

Co-authored-by: Dianeod <40861871+Dianeod@users.noreply.github.com>
Co-authored-by: Diane ODonoghue <dodonoghue@kx.com>
Co-authored-by: Conor McCarthy <conormccarthy@brainpool1.mynet>
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.

2 participants