Strategies are `.pyx` only ? about gryphon

[rgdagir]

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 gryphon-exec strategy mystrat (no .py at the end). Should work.

[garethdmm]

The code that loads strategies is right here:

gryphon/gryphon/execution/live_runner.py

Lines 146 to 161 in 8cd2f43

	def get_strategy_class_from_filepath(strategy_path):
	"""
	Load a strategy that the library user has written and is specified as a filepath
	from the current working directory to a .pyx file.
	"""
	module_path = strategy_path.replace('/', '.').replace('.pyx', '')
	# Since in this case we're importing a file outside of the library, we have to
	# add our current directory to the python path.
	sys.path.append(os.getcwd())
	module = importlib.import_module(module_path)
	strategy_class = get_strategy_class_from_module(module)
	return strategy_class

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
gryphon-exec strategy strats/mystrat -> SUCCESS
gryphon-exec strategy strats/mystrat.pyx -> SUCCESS

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.

[asmodehn]

importlib already has a way to import a module from a filepath.
So I wouldn't want to add any logic on top of that.
Meaning :

• argument is a file path, load the file path with importlib.
• argument is a module name, load the module name with importlib.

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)
So I guess this might depend on gryphon's expected minimum python version ?

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.

[garethdmm]

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!

[asmodehn]

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> .

[asmodehn]

I believe the fix left this part aside :
https://github.com/garethdmm/gryphon/blob/master/gryphon/execution/lib/config_helper.py#L96
Which means the configuration file should be named with "strat_name.py.conf" currently, and that is probably not intended