-
Notifications
You must be signed in to change notification settings - Fork 93
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
Move the logic from Cylc commands/scripts to the cylc package #2802
Comments
Agree that most logic in |
Also agree - everything above sounds good to me. We could do it now, if you think it wouldn't delay us too long? |
I'm pondering whether I should give it a try while we have some time before the work on Python 3 on isodatetime/Cylc. Will choose a command that I am familiar like |
While reviewing #2809 I found that Here's a simplified example of what it does: import os
from subprocess import Popen, PIPE
cmd = ['cylc', 'run', 'suite', '--no-detach', '--verbose']
proc = Popen(
cmd,
stdin=open(os.devnull), stdout=PIPE, stderr=PIPE)
if proc.wait():
print("Error executing command: %s" % proc.communicate()[1])
else:
comm = proc.communicate()
print("Done! %s\n%s" % (comm[0], comm[1])) If not for the from cylc import scheduler_cli
import sys
cmd = ['run', 'suite', '--no-detach', '--verbose']
sys.argv = cmd
scheduler_cli.main() I used a dummy suite called suite.
It finished in approximately 3.5 seconds for both scripts, but I suspect the Python version would be faster if the system is a bit more busy (and thus lacking resources to immediately start a process, due to memory/cpu/etc). But one advantage of the second option, is that we have more control over what is going on, and can investigate issues and optimize the code more easily. Here's the call graph for the In this graph, you can't really see what's going on after the process was started. The only calls you have are the trigger of the process, and then OS and low level calls to check the status of the process. Here's what it looks when calling the Python code directly. Here it is clear that the script And from the statistics (I am using the Profiler bundled with PyCharm) one can see 53.5% of the time spent waiting ( But it is much easier to see and debug it without multiple processes. There could be implications on how Cylc currently works, that could prevent us of moving in this direction. |
Interestingly
|
@hjoliver I think the I have a piece of paper I was tracking down some calls in a suite at work. I can confirm tomorrow, but I think I had a branch somewhere experimenting with That's why I kept this part here. I think the work on #2809 is all good, as far as I could tell. It works like the other parts of the system. I am still planning to test it in my local environment with a Docker container, then send a +1 ⭐ Another by-product of this test, I hope, will be an experiment - or maybe an example - to show how to use Docker with Travis-CI. This way we could have tests with real remote environments if necessary. |
(but re-reading the issue description here, maybe it would be better to put it in another issue, sorry) |
@kinow - OK understood - I was just taking your statement too literally! ( |
Note to self: if there is some sort of common API for the commands, it would be nice to add a simple try/catch for |
Hi @kinow |
But I think it prints the error in the consolr output after you press ctrl+c. Or another one does that. I noticed some commands catch KeyboardInterrupt and exit nicely, while others exit and also let the user see the errors in the terminal |
Yes, fully agreed on consistency with Just re-reading some of the previous comments... On The biggest commands are:
|
Following the discutions on #3393, about going with entry_points and a central command with subcommands, it looks like click could be used for that but that would require a multistep migration. That would keep the current workflow as is in terms of implementation. In a second phase, move the cylc script back to python using click and then make all the other commands sub commands of the main click application. |
This sounds like a plan! Are you interested in working on a pull request for this? For personal projects I use click as I find it easier to use. But it would be good if we could implement this first without throwing away the argpase parts. Would it be ppssible too @sgaist? |
@sgaist - if you've gone back to the original cylc python script from several years ago, we'll need to be a) careful about changes made since then in the newer bash script; and b) take a very close look at performance in light of comments above. That script is pretty ancient, and probably not well coded. (On the other hand, it is fundamentally quite simple, so it should be easy to streamline it). |
For that statement, I have no opinion either way. Just want to provide extra context. The reality is that apart from the occasional For simplicity (and for the future Python API), I fully support moving towards a pure Python system. |
Let us know what the issues are, I've fixed a couple of simple Mac OS bugs in the past, we aren't great at sticking to POSIX. Couple of installation notes:
|
The old script may be good for a proof of concept but there have been a few changes, also the old Python script wasn't great. |
That's a consensus then 👍 back to Python for |
As requested, the branch is https://github.com/sgaist/cylc-flow/tree/move_to_entry_points I have made a minimal cylc script using click since it seems to be one of the possible future framework for building the command set. Note that it's not optimal currently as for example click provides some tools to handle abbreviated commands. On a side note, did you consider using pre-commit for checking/fixing code before committing ? There are some nice tools like black, bandit, flake8, etc. that could be run directly on the developer machine and might help keeping the code base tidy without having to redo commits. |
Might be a good idea the pre commit hooks. JupyterHub and/or pandad I think had some. Feel free to create an issue/PR for it :) |
(Working on the frontend today, but will take a look at the branch as soon as I'm done over there. Thanks @sgaist!!!) |
@sgaist - good idea on the pre-commit hooks. We do run bandit, and style checking, etc., via codacy and github, but errors revealed there do require "redoing commits" as you say. 👍 |
@sgaist - I took a quick look at your branch. Initial comments:
Some current CLI help functionality is broken compared to master, but presumably easy to fix (or potentially, we could change to a new way):
Some commands (at least one) missing?
Sounds like you could use Seems like very minor problems. @cylc/core - if others agree, shall we suggest @sgaist puts up a PR from his branch? |
Great work @sgaist ! On the package name, what about
I think it would be better to postpone it. We had discussed The
+1 from me. |
Just tested the code, Soon I might be able to invoke some of the Cylc subcommands from Jupyter notebooks :) |
👍 |
Click is nice, but we have recently decided for Clize in metoppv/improver#955 for our CLIs if you want to see how it looks like. The main attraction for us is integration with Sphinx (needed some hacking to support Google-style docstrings though). |
Cool, please raise a PR when ready.
Not keen on this personally but if you're all convinced, ignore me.
As far as I understand click is just syntatic sugar to optparse so fine. Just two requirements I'd like to flush out first:
👍 and shift |
I was about to ask just that, how did you get Google-style docstrings to work? |
By subclassing ClizeHelp to insert docstring conversion in the right place: |
Nice, you should push that to epsy/clize. |
Arg parser comparitive notes: https://clize.readthedocs.io/en/stable/alternatives.html After an admittedly pretty superficial look, I vote for click. The decorators are cool. It is also more active as a project. A quick google for sphinx integration found this: https://github.com/click-contrib/sphinx-click (@oliver-sanders or @sadielbartholomew better to judge than me though, on this). |
(So much for The Zen of Python.)
|
I guess that only applies to the core language 🤣 |
Might work well as you probably have different goals for Cylc. One of the things we wanted to achieve is 1-2-1 mapping between our top level python functions APIs and CLIs. This wouldn't work if we can't use "CLI functions" as normal python functions at the same time (i.e., pass non-string arguments to them). |
@sgaist - if you can reassure us on the two check-box concerns discussed above #2802 (comment) then please go ahead and post a PR, but back out your commit of black-processed files (which is still not agreed for Cylc). Also, if it is the case that click requires very minimal coding (as appears to be the case on your branch) we could easily swap it out again if necessary, before Cylc-8 is released (mid 2020), so that's not a big deal I think. |
About the missing commands, there currently are three:
These are shell scripts that must be ported back to python but I wanted to have a proof of concept working before doing the port. I haven't tested click-sphinx but it looks exactly what would be needed. One thing I'd like to also note is that the current cylc script is just forwarding calls further mostly like the original shell script did. I have done it like that for the proof of concept. If click is deemed the way to go, all the other commands could be ported to click as well and then plug in as sub-commands. That way help and parameters handling would be forwarded to the correct command. It will also allow for other cylc projects to add new command without having to modify the main package to support them. For example, the command_list generated at beginning of the script can be avoided when all commands are implemented through click. |
Ah, roger that.
That would indeed be good (and what I was getting at with "Sounds like you could use click more extensively?" above) - can be done as a follow-up to the initial PR. |
Closed by #3413 |
For reference: python packaging - Command Line Scripts
I have a branch with a work-in-progress
setup.py
for Cylc (if you have a look at the branch, skip the deleted files and don't mind the number of commits).In the
setup.py
, we have the new list of dependencies for Cylc, likeisodatetime
, andjinja2
, matching versions required. And there is also an entry forscripts
. Thescripts
option is configured to list all files under thebin/
folder.What it means, is that running
pip install cylc
(not exactly as we haven't published anything there... but just for example) would install the modulecylc
(i.e. contents oflib/cylc
), its dependencies (e.g. versions needed forjinja2
, andisodatetime
), and also put the Cylc scripts in the$PATH
.Right now users are asked to do that as a part of the installation process, but with this approach that won't be necessary anymore. The scripts would be installed with the new module in the user environment.
Furthermore, users would be able to create
virtualenv
's,conda
environments, or if the deb/rpm packaging works, they could use that option too. And that way they would be able to manage multiple versions of Cylc.It is also possible to achieve the same with containers, but containers are at a higher level. The option with
pip
requires simply a Python installation,pip
, andvirtualenv
to support multiple versions (or in the supercomputer perhaps thatload module
trick...).One problem with this approach, however, is that it becomes clear that the scripts under the
bin/
folder contain too much logic.This ticket suggests to move that logic to the Cylc module (i.e. under
lib/cylc
), within appropriate objects for encapsulation. Write tests (which is harder right now), and then later replace thescripts
by the Python entry point:console_scripts
.The
console_scripts
allow to expose functions as scripts.In other words,
cylc scan
would be a thin shim, calling actually a function likeSomeCylcObject.scan(*args)
.The clear downside is the complexity and risk of this change.
But here's a list of benefits:
import cylc
in a Jupyter Notebook, for example, would be able to write code that calls that same function, without having to fork a processcylc scan
or any other command (Probably not entirely true... we could still re-use themain() method
in most cases?...)This can be done after the new Web GUI. I would prefer to try to implement this before, as I think the code would look simpler... but on the other hand, if we are not able to implement it right now, I could benchmark the current approach with the scripts, and compare against this approach and validate the last item.
Cheers
Bruno
The text was updated successfully, but these errors were encountered: