-
Notifications
You must be signed in to change notification settings - Fork 151
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
Strategies are .pyx
only ?
#30
Comments
I had a similar bug. I don't know how the runner is parsing the strategy argument, so I just cd'ed into the strategies folder and ran |
The code that loads strategies is right here: gryphon/gryphon/execution/live_runner.py Lines 146 to 161 in 8cd2f43
Looking at it now, yes it does expect .pyx and not .py. and because of the way importlib works, it will work if you don't give a filetype suffix. So: gryphon-exec strategy strats/mystrat.py -> FAIL Seems to me the simplest fix here would be to truncate both .py and .pyx suffixes from any strategy file and then let importlib succeed/fail on it's own. That will let users use .py or .pyx on their own whim. On the other hand, maybe we want to enforce one or the other filetype so that users get the biggest performance benefit possible from cython. What do you guys think? I'm open to options. |
importlib already has a way to import a module from a filepath.
However it can be quite troublesome to support between different python versions (see https://stackoverflow.com/questions/67631/how-to-import-a-module-given-the-full-path) If backward compatibility is needed, I made a while ago for other projects a backport of the python3 import logic into python2 : https://github.com/asmodehn/filefinder2, but that might add extra complexity that is not really needed, since python2 is dying already. |
Definitely looks like there's considerations with importlib as we move towards python3, but I think we can consider that as part of the python3 project. I think the initial bug you identified, that loading strategies from .py files is broken, is big enough to fix immediately, and the fix is pretty simple: we just add one more 'replace' call on line 152 to strip '.py' suffixes as well as '.pyx'. Do you want to make that edit and create a PR? You identified the bug so feels appropriate. If not I can do it too, just thought I'd offer! |
No problem, I ll do the quick fix soonish ;-)
…On Sat, Jun 29, 2019, 18:57 Gareth MacLeod ***@***.***> wrote:
The only logic I was suggesting adding was to add '.py' to the list of
suffixes we strip from paths.
Definitely looks like there's considerations with importlib as we move
towards python3, but I think we can consider that as part of the python3
project.
I think the initial bug you identified, that loading strategies from .py
files is broken, is big enough to fix now, and the fix is pretty simple. We
just add one more 'replace' call on line 152 to strip '.py' suffixes as
well as '.pyx'.
Do you want to make that edit and create a PR? You identified the bug so
feels appropriate. If not I can do it too, just thought I'd offer!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#30?email_source=notifications&email_token=AABWASB3DMT36XYK3GWLHJLP46H7LA5CNFSM4H4DEBR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY34NLA#issuecomment-506971820>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABWASGNOV6XSS7QMJLLWUTP46H7LANCNFSM4H4DEBRQ>
.
|
I believe the fix left this part aside : |
Is that expected ?
Attempting to run a
gryphon-exec strategy strategies/mystrat.py
gives :It looks like it might be an easy fix...
The text was updated successfully, but these errors were encountered: