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

Yan 968 #143

Merged
merged 36 commits into from
Apr 22, 2022
Merged

Yan 968 #143

merged 36 commits into from
Apr 22, 2022

Conversation

awicenec
Copy link
Contributor

No description provided.

awicenec and others added 30 commits April 6, 2022 14:50
Fixed travis badge link
tested nifty graphs
…cognise additional parameter types basedon default value
…arsing of parameters. Skip components whose methods begin with an underscore.
Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
We have been getting away with this code, but in reality we have always
been guilty of not taking the GIL in the "pythonic" dynlibs (i.e., those
offering a "init2" method rather than an "init" one). Until 3.8 no
errors have showed up, but in 3.9 segfaults start to happen when calling
PyLong_FromLong (and possibly other methods).

To solve this I've updated our example code to take the GIL as
appropriate. This is verbose, but it's the simples solution. A different
approach would have been to load the dynlib with ctypes's PyDLL rather
than CDLL, but that forces all C function calls to hold the GIL even if
not required, which is less performant. Nobody (except Kevin, if he
still does?) is using this anyway, and if so they would have run into
the same error as we did here, so there's no issue in clarifying that
the GIL must be taken by the dynlib code rather than by daliuge.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
This was found while stabilising the unit tests running against python
3.9, which in turn I was doing to have a stable basis to work against
for YAN-968.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
Now that the problems with 3.9 have been solved we should be able to
test against it without problems.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
This was found while working on YAN-968.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
When creating drops from a graph specification we used to use only the
top-level key-values to build the keyword map passed down to
applications, which then inspected it to obtain their user-provided
arguments. For some time now LGs (and their translation into PGs) have
separated how arguments are provided to applications though, grouping
them together inside an applicationArgs map instead of placing them at
the top-level of each drop specification. This means that applications
cannot access these values directly, and that the mechanism to declare
arguments at the class level (using dlg_string_param and friends)
doesn't pick them up.

This commit merges the information contained in applicationArgs into the
set of arguments passed down to drops via kwargs. This is enough for the
dlg_*_param declarations to pick these up.

This is part of YAN-968.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
This is part of YAN-968.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@coveralls
Copy link

coveralls commented Apr 21, 2022

Coverage Status

Coverage decreased (-0.1%) to 80.661% when pulling 6946e7e on yan-968 into b9e1148 on master.

@awicenec
Copy link
Contributor Author

This pull request covers a few smallish changes which are related with the yan-968 ticket. Some part of which had been fixed by Rodrigo in a separate ticket and already merged. There are also a few other changes from different branches, which might have an influence on this and thus I've merged master back into this branch already and it looks like there are more changes than what's actually happened.

This seems to be what is in the code when I checked out the branch, so I'm confused why the code in the PR was different.
Copy link
Collaborator

@pritchardn pritchardn left a comment

Choose a reason for hiding this comment

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

This doesn't appear to break anything user-facing - should be fine to merge.

@awicenec awicenec merged commit c128d59 into master Apr 22, 2022
@awicenec awicenec deleted the yan-968 branch April 22, 2022 02:26
awicenec added a commit that referenced this pull request May 19, 2022
pritchardn pushed a commit that referenced this pull request May 20, 2022
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.

None yet

6 participants