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 API: general improvements #30

Closed
yoogx opened this issue Apr 20, 2015 · 17 comments
Closed

Python API: general improvements #30

yoogx opened this issue Apr 20, 2015 · 17 comments
Assignees

Comments

@yoogx
Copy link
Contributor

yoogx commented Apr 20, 2015

This issue is to track request for improvements: which function should return something?

  • analyse() should return a boolean

others?

yoogx added a commit that referenced this issue Apr 20, 2015
@maxime-esa
Copy link
Contributor

watch out, Makefile.am in resources/runtime/python/ has a typo line 6 ( + = instead of +=)

JH: corrected, thanks

@maxime-esa
Copy link
Contributor

TODO: in ocarina.py, add some warning to the user if a module cannot be imported, instead of catching ImportError and discarding it

@yoogx yoogx changed the title Python API: return parameter of function Python API: general improvements Apr 29, 2015
yoogx added a commit that referenced this issue May 16, 2015
        Fixes regression for properties like Transmission_Time

        For issue #30
@maxime-esa
Copy link
Contributor

Issue in function analyze: the return values are not understood (and wrong):

this:

def analyze ():
    '''Analyze models'''
    info = io.BytesIO()
    error = io.BytesIO()
    raisedError = []
    res = ''
    with std_redirector(info,error):
        try:
            res = libocarina_python.analyze ()
        except:
            raisedError.append(getErrorMessage()) 
    stderrMsg = sortStderrMessages(error.getvalue().decode('utf-8'))
    if stderrMsg[1]!=[]:
        raisedError.append(stderrMsg[1])
    return [ res , info.getvalue().decode('utf-8'), stderrMsg[0] , 
        raisedError ]

returns this:

res= True
info= Model analyzed sucessfully

err= []
raised= [[u'InterfaceView.aadl:56:07: Warning: Source_Language is not a list while the corresponding property name at programming_properties.aadl:61:02 is a list.\n\nInterfaceView.aadl:56:07: Warning: The value of Source_Language has been converted into a list.\n\nInterfaceView.aadl:87:07: Warning: Source_Language is not a list while the corresponding property name at programming_properties.aadl:61:02 is a list.\n\nInterfaceView.aadl:87:07: Warning: The value of Source_Language has been converted into a list.\n\nInterfaceView.aadl:112:07: Warning: Source_Language is not a list while the corresponding property name at programming_properties.aadl:61:02 is a list.\n\nInterfaceView.aadl:112:07: Warning: The value of Source_Language has been converted into a list.']]

The first value (res) is OK - General output status
The second (info) is a string - corresponding to the general status - OK, why not
but then err and raised (called raisedError in ocarina.py) are not clear
Why is the last argument of type [['long string with all warnings']] ?

Thanks

@maxime-esa
Copy link
Contributor

Not sure what you are doing with stderr in the std_redirector context manager, but this messes up the logging functionality from stdlib and it becomes impossible to use it after calling an ocarina function:

Traceback (most recent call last):
  File "/usr/lib/python2.7/logging/__init__.py", line 880, in emit
    stream.write(fs % msg)
ValueError: I/O operation on closed file

This is probably due to the sys.stderr.close() that you do (in ocarina_common_tools.py), but I can't tell for sure.
Perhaps related to this discussion: http://bugs.python.org/issue6333

@maxime-esa
Copy link
Contributor

The return value of the instantiate function seems wrong:

success, _, _, _ = ocarina.instantiate("interfaceview.others")

The first return value (called success here) was expected to be a boolean, but it is a empty string.

@Ellidiss
Copy link
Contributor

Regarding the output of the API functions, here is a part of the to be published design report:

All the functions available in the API have the same template for their return value.
It is a list of the form { [result, empty string if none], [stdout from Ocarina], [warnings from ocarina], [errors from Ocarina]}
which allows an homogeneous management of error.

In stderr, everything that is not considered as warning is considered an error.
In your exemple, the warnings are prefixed with the aadl file name. In the API, to be considered as warning, a line in the stderr shall start with "warning:". Has the warning output been modified or do I missed something?

Regarding the context manager, in order to get stderr and stdout, both channels are redirected to two new python streams. Seems that I forget to replug stdout/err to the old channels after the function is called. Will look at it.

Regarding the type of the result, it has not been defined to be a boolean. I just forward the result gived by Ocarina as a string. If the awaited result is false when error occurs and true in other cases, just let me know.

@Ellidiss
Copy link
Contributor

Regarding the logging functionnality of the libc, in the std_redirector, the stdout/err are repluged to the original channels at the end of the ocarina function call.
Does the logging facility open a channel on the stderr prior to call the function in Ocarina and uses it after the call return? If that is the case, yes, the call to sys.stderr.close() may break the channel from the logging facility.

@maxime-esa
Copy link
Contributor

Regarding the type of the result, it has not been defined to be a boolean. I just forward the result gived by Ocarina as a string. If the awaited result is false when error occurs and true in other cases, just let me know.

In the example shown above, "analyse" seems to return a boolean, not a string (res = True), while the 2nd return value is the string returned by ocarina (Model analyzed successfully).
I think it is better like this - what other string could be emitted in the first return value?

@maxime-esa
Copy link
Contributor

It is a list of the form { [result, empty string if none], [stdout from Ocarina], [warnings from ocarina], [errors from Ocarina]}

But this is not what is actually sent.

  1. Typewise, what you send is:
[str (or bool), str, list, list]

and NOT

set(list, list, list, list)    # {} is for set , [] is for list in your above interface  
  1. Check the example above, semantically speaking it is also not consistent: you say the last parameter is the list of errors from ocarina, but in fact it is the warnings (and the warnings list is empty)

  2. You send lists (for warnings and errors) but in fact you have only one element which is a single string. You should split the string into a tuple/list- using "warning:" to split.

@Ellidiss
Copy link
Contributor

Maybe I used a wrong style, I really wanted to say the result is a list of four elements which are:

  • a string
  • a string
  • a list of string
  • a list of string.

Regarding the warnings, I mentionned the problem of the signature of the warning message four comments above.
In the current code, I consider that a warning line starts with "warning:" whereas in your exemple it seems to start by an AADL file name followed by a line and a column number and only after that by "warning:". Then I consider all the stderr lines to be error messages. That explains the content of the result (no warnings in the warning "list" but a lot of warnings in the error "list").
For the type of the two last elements of the result, I agree that it is a bug.
Now, what I need to know is the signature of a warning message written by Ocarina in stderr (Jérôme?).

JH: what do you mean by signature ? all warning messages should be prefixed by "warning", if not open a ticket

@Ellidiss
Copy link
Contributor

Regarding the first item of the result list of the API function.
Its underling type depends on the called Ocarina function. For instances:

  • libocarina_python.getComponentName returns a string then res is a string
  • libocarina_python.analyze returns a boolean then res is a boolean
  • libocarina_python.instantiate does not return a value then res is an empty string
  • libocarina_python.getInstances returns a list of nodeid then res is a list of integer
  • ...

As such, this item cannot be considered neither as a boolean nor as a string. Its type is context dependant.
Except if there is a specific requirement, I do not know what should be changed.

@maxime-esa
Copy link
Contributor

(Topic: First returned item)

OK to be context-dependent. In the case of instantiate I thought that for consistency it should return a boolean but if the inconsistency is in Ocarina and not in your part, I guess there is nothing you can do (But Jerome: I would suggest to return a boolean for this function, just like analyze)

JH: not a problem to return a boolean, you asked it also for analyze in the past (see the history). This is now implemented, but not tested. There might be trivial bug to solve

@maxime-esa
Copy link
Contributor

Regarding the logging functionnality of the libc, in the std_redirector, the stdout/err are repluged to the original channels at the end of the ocarina function call.
Does the logging facility open a channel on the stderr prior to call the function in Ocarina and uses it after the call return? If that is the case, yes, the call to sys.stderr.close() may break the channel from the logging facility.

I am not sure how the logging function is implemented, but I think it is something like that.
When you use it, you initialize it at the begining of the application:

log = logging.getLogger(__name__)
console = logging.StreamHandler(sys.__stdout__)
console.setFormatter(ColorFormatter())
log.addHandler(console)

And there anywhere in your application you can do:

log.info('hello')
log.warning('world')
log.error ('....')

So I guess that if you close stdout and stderr in the middle of the application, the logging cannot recover the channel...

@maxime-esa
Copy link
Contributor

Logging: I found a workaround, by using a StringIO in the logger, and print it at the end of the application:

Initialization of the logger:

    log = logging.getLogger(__name__)
    stream = StringIO.StringIO()
    console = logging.StreamHandler(stream)
    console.setFormatter(ColorFormatter())
    log.addHandler(console)

and before the application exits:

    print stream.getvalue()
    stream.flush()
    stream.close()

yoogx added a commit that referenced this issue Jun 24, 2015
yoogx added a commit that referenced this issue Jun 24, 2015
@Ellidiss
Copy link
Contributor

(Topic: signature of error/warning)

I found two signatures for warning messages:

  • warning : [warningMessage]
  • [aadlFile] : [lineNumber] : [ columnNumber] : [warningMessage]

I change my code to take the two versions. But if you confirm that it is a bug, I will create a ticket.

JH: It is not, in some occasion we do not have access to the line number. But I guess there is still a minor bug: for the second occurrence, there should also be "warning" printed. Can you open a ticket with an example ? Thanks

@maxime-esa
Copy link
Contributor

@yoogx : the "Warning" is printed, as seen in example above:
InterfaceView.aadl:56:07: Warning: Source_Language is not a list while the....

@yoogx yoogx self-assigned this Jun 26, 2015
@yoogx
Copy link
Contributor Author

yoogx commented Apr 17, 2016

Closing this old ticket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants