Pyfunc code #261
Pyfunc code #261
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @awicenec - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
The problem is that it could be a base64 encoded string, which first needs
to be translated into a bytes object. The case that it might be a base64
encoded string which is actually directly a code object is not covered. I
think a few of the tests are doing something wrong and that's what
triggered me to use the try except with the two exceptions, because one did
not cover all the tests. The SyntaxError ones are the ones which are coming
in as strings. I will add a bit of docstring as well.
…On Thu, 30 May 2024, 17:25 Ryan Bunney, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In daliuge-engine/dlg/apps/pyfunc.py
<#261 (comment)>:
> @@ -31,6 +31,7 @@
import logging
import os
import pickle
+import pyext
This module hasn't been updated for quite some time: 9 years according to
https://github.com/refi64/PyExt.
It also looks like there's a version mismatch between PyPi and the master
branch on the repository (Pypi is 0.6, repo is 0.8)?
https://github.com/refi64/PyExt/blob/1e018b12752ceb279f11aee123ed773d63dcec7e/pyext.py#L25;
https://pypi.org/project/pyext/#history.
It's such a small repository we could always fork it and use it within our
code.
—
Reply to this email directly, view it on GitHub
<#261 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJVMSCMXJI5EU62GKEL65LZE3V7XAVCNFSM6AAAAABIOYJ74CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOBXG42DSNJYG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
- Fixed error in app_base.py where the incorrect track_current_drop was being used. - Added unit test to make sure this regression doesn't occur in the future - Improved logging of exceptions in session.py
This is the result of not testing after cleaning up code whilst preparing a commit...
Hopefully this will improve the likelihood of the tests passing on GitHub. Also removed methods used during testing that are no longer of use.
- Also added the `conftest.py` so we properly load the runtime when starting pytest as a suite (as opposed to running invidually).
- Can now specify --rpc_port and --event_port. - Verified this works and two NodeManagers can be run using localhost.
| @@ -533,7 +533,7 @@ class ArrayGatherApp(BarrierAppDROP): | |||
| [dlg_batch_output("binary/*", [])], | |||
| [dlg_streaming_input("binary/*")], | |||
| ) | |||
| value_list = dlg_list_param("value_list", []) | |||
| # value_list = dlg_list_param("value_list", []) | |||
There was a problem hiding this comment.
This value is used in the readWriteData method; I recognise that the run is called before readWriteData, but we should either initialise it earlier, or enforce that readWriteData can't be called before run().
| from dlg.manager.client import MasterManagerClient | ||
| from dlg.manager.proc_daemon import DlgDaemon | ||
|
|
||
| _TIMEOUT = 5 | ||
| _TIMEOUT = 3 |
There was a problem hiding this comment.
3 is likely too low - I have success getting this to work locally with 10 second timeout (which I know you previously tried here).
There was a problem hiding this comment.
Yes, I tried 10s, but it did not change anything, except that the time you had to wait until the tests showed up as failed got incredibly long. The original value was 5s and I will change it back to that. There are actually two timeouts, one for the startup of the manager (this one) and another for the client to try to connect to the manager (10s). Usually that's the one that fails, but that is not the actual problem, the problem is that the manager does not start up for some reason. Locally I never have issues, since the startup is much faster than even 3s.
|
This is now a merge of three branches LIU-395, LIU-396 and pyfunc_code into master. |
Pyfunc code + LIU-395 and LIU-396
This PR covers the very important and useful change to allow users to enter a function definition as a plain string directly into the func_code field of the PyFuncAPP. Up to now this required the string to be base64 encoded. For backwards compatibility reasons that is also still supported. The package pyext is used to generate a module from a code string at runtime. The code string can be single and multi-line, best a copy-and-paste from an editor. There is only one restriction: The function has to be defined as 'f', i.e.
and the arguments have to be defined in the Fields Table for that function.
Note that this functionality had been regarded as a security risk before. Since the engine is usually running in user space on a resource where the user can run arbitrary Python code anyway, it does not directly add an additional risk. However, if the user would run a graph from an unknown source without checking the func_code entries that could cause an issue, but not a bigger issue than installing code from an unknown source.