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

Make cylc a module #2990

Merged
merged 32 commits into from
Apr 25, 2019
Merged

Make cylc a module #2990

merged 32 commits into from
Apr 25, 2019

Conversation

kinow
Copy link
Member

@kinow kinow commented Mar 12, 2019

Fix #2989

Adapted code from previous pull request, but it was a bit outdated (4 months old), with conflicts due to Python 3. So I thought it would be easier to copy the setuptools files, and simply apply the other changes manually.

Also looked at isodatetime to see what we could improve in our setup.py.

  • Based on master with Python 3
  • Python requires set to 3.7
  • Removed the folders of jinja2, markupsafe, and isodatetime, as they are not dependencies in setup.py
  • Added dependencies introduced for Python 3 (e.g. colorama, pyzmq) to setup.py
  • Moved static metadata to setup.cfg. It is possible to confirm what it looks like running rm -rf lib/cylc.egg*, then pip install -e ., and inspecting the PKGINFO file created within that egg folder
  • Copied the topics from our GitHub repository as keywords (except for Python)
  • Added more classifiers to the setup.cfg
  • Updates .travis.yml file to run pip install -e .[all] and pip install -e ..
  • Removed the old ssl groups from the old pull request, as it does not seem to be a requirement for pyzmq (@oliver-sanders ? not sure if I missed any dependency for Python 3)
  • Added dependencies for documentation, and had to remove cylc-make-docs and replace by setuptools and Sphinx integration due to CYLC_DIR use

Tests were done via python setup.py test and cylc run --no-detach --debug --verbose five. Changes passing in Travis-CI.

Dependencies were frozen, so we don't run into build/security problems due to unintentional updates in environments.

colorama==0.4.1
isodatetime==1!2.0.0
jinja2==2.10
markupsafe==1.1.1
python-jose==3.0.1
pyzmq==18.0.1

[all]
EmPy==3.3.4
sphinx==2.0.0
codecov==2.0.15
coverage==4.5.3
pytest-cov==2.6.1
pytest==4.4.0
pycodestyle==2.5.0
testfixtures==6.6.2

[docs]
sphinx==2.0.0

[empy]
EmPy==3.3.4

To see available commands, try python setup.py --help-commands. It works similarly to cylc help or cylc-check-software. With the difference that new commands may appear if you install plugins.

When you clone the repo, for instance, you won't see build_sphinx, unless you have the sphinx dependency. If you run pip install -e .[all] or pip install -e .[docs] (i.e. install editable with all or docs dependency group) then the build_sphinx option should automagically appear as available in python setup.py help-commands.

Related: #2834

@kinow kinow added this to the cylc-8-alpha-1 milestone Mar 12, 2019
@kinow kinow self-assigned this Mar 12, 2019
@kinow kinow marked this pull request as ready for review March 12, 2019 02:00
@kinow
Copy link
Member Author

kinow commented Mar 12, 2019

I now realize that the discussion in the WIP pull request, if done in a ticket, it would still be assigned to this milestone. Instead of lost in the closed pull request.

Something to improve :-) even though I may still use WIP/Draft pull requests, will create a placeholder ticket for discussions, important notes, links, etc. 👍 apologies. At least tried to link with a footnote comment in this issue description...

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@oliver-sanders
Copy link
Member

oliver-sanders commented Mar 12, 2019

Great, I've wanted this done for ages! Now it's all happening so fast.

Removed the old ssl groups from the old pull request, as it does not seem to be a requirement for pyzmq (@oliver-sanders ? not sure if I missed any dependency for Python 3)

Yep, no more ssl!

Tests were done via python setup.py test and cylc run --no-detach --debug --verbose five. Let's see if Travis-CI approves these changes too.

I think you might get a test failure due to empy, perhaps comment it out of the setup.py until the conclusion of #2958 .

@kinow
Copy link
Member Author

kinow commented Mar 12, 2019

Excellent review as always @oliver-sanders ! Have updated a few items, but will go through tox, and the other pending ones with more calm tomorrow morning. Thanks!

setup.cfg Outdated Show resolved Hide resolved
@kinow
Copy link
Member Author

kinow commented Mar 12, 2019

@oliver-sanders

I think you might get a test failure due to empy, perhaps comment it out of the setup.py until the conclusion of #2958 .

There is an extra dependency group [empy] that requires EmPy. For running unit tests, instead of simply pip install -e ., it's simply doing a pip install -e .[all]. The all group includes empy (and included ssl pre zmq).

@kinow
Copy link
Member Author

kinow commented Mar 20, 2019

Note: In some Python projects, the structure includes a folder with the same name of the project, that contains the source code. e.g. theproject/theproject. So in setup.py something like from theproject import __version__ works, as it can find the theproject local package, relative to setup.py.

In our case, we have cylc/lib/cylc. So importing it does not work, unless we hack sys.path (not always recommended). Instead, other projects use the approach mentioned in this documentation of Python packaging, which parses the __init__.py file.

Also note, that having __version__ is documented in PEP-396.

@kinow kinow force-pushed the make-cylc-a-module-take-2 branch from 2f56631 to bbdb04e Compare March 20, 2019 05:38
@kinow
Copy link
Member Author

kinow commented Mar 20, 2019

The build in Travis for the functional tests took only 2 minutes each job. Looks like there's something wrong with an import somewhere. Getting an error running it locally.

@kinow
Copy link
Member Author

kinow commented Mar 20, 2019

Found the issue. At the moment, several scripts (Python and Shell) depend on the CYLC_DIR variable. So far, the occurrences I found rely on cylc version --long, to retrieve the location of CYLC_DIR - possible to exist other ways of setting this variable.

In my environment I have Anaconda Python 3.7, with a virtual environment activated for Cylc (to isolate other projects, e.g. cylc-jupyterhub, from crashing due to dependencies installed).

Running cylc version --long is returning: Cylc 8.0a0 (/home/kinow/Development/python/workspace/cylc/venv).

Then the scripts use the venv folder as basis, and everything goes wrong after that. Will think about this issue in the morning after some good strong coffee ☕ :)

@hjoliver
Copy link
Member

We can restructure the source tree if necessary, to make it more standard and better for packaging.

@kinow
Copy link
Member Author

kinow commented Mar 20, 2019

We can restructure the source tree if necessary, to make it more standard and better for packaging.

It should still work with the lib/cylc. The issue now is that within virtual environments, the current approach for getting CYLC_DIR is broken, as the scripts are installed in the $PATH, not in a folder like /opt/cylc and found via usr/bin/cylc script any longer.

Will try to find a solution that changes as little code as possible first. If that doesn't work, then it may require changing how CYLC_DIR works (or possibly remove it).

@hjoliver
Copy link
Member

BTW I vaguely recall we talked about packaging and multiple cylc versions (?) but can't recall the outcome, sorry.

ATM users can run different suites at different versions if necessary (e.g. long-running suite that's not upgraded yet, and a new suite written for new cylc) and every suite job invokes the right cylc version thanks to $CYLC_VERSION in the job environment, with cylc versions installed in (e.g.) in /opt/cylc/[VERSION].

Can we still support this sort of thing without users having to pip install their own cylc versions into their own virtual environments?

@kinow
Copy link
Member Author

kinow commented Mar 20, 2019

Can we still support this sort of thing without users having to pip install their own cylc versions into their own virtual environments?

I saw it somewhere while reviewing the Python3 pull request, but can't recall which file. I will update the issue description with a list of requirements to consider cylc module is "working".

@hjoliver
Copy link
Member

For sites that need this capability, maybe we could just explain how to install multiple virtual environments centrally (one for each cylc version) and have the central cylc wrapper script load the version-specif environment instead of just pointing to the right bin path as it does now?

@kinow
Copy link
Member Author

kinow commented Mar 20, 2019

If that option would allow the operations guy to keep old suites, while using Cylc 8, I'd be +1 for that as it's simpler.

@hjoliver
Copy link
Member

👍 @matthewrmshin seems to agree, so let's go for that.

@kinow kinow mentioned this pull request Mar 21, 2019
@kinow kinow force-pushed the make-cylc-a-module-take-2 branch from bbdb04e to 6de6b47 Compare March 21, 2019 02:27
@matthewrmshin
Copy link
Contributor

Environment manipulation can be done in many ways. Some modern ways may be very sophisticated, but may also be slow.

The most important thing is to make sure that we have efficient logic for calling cylc message, etc - and also isolate users from their own environment with what is required by Cylc.

@matthewrmshin
Copy link
Contributor

(I am happy as long as the new way is nice and efficient.)

@hjoliver
Copy link
Member

@cylc/core - can we try to get this merged this week or next week? (depending on UK team availability late April I guess) ... it's been a long wait. It will make some other things easier, and details such as dependency version specificity can always be revisited later.

@kinow is there anything much remaining to be resolved on this that you're aware of, from reviews so far? Do we need to move non-Python files (example suites as discussed above, and editor syntax files etc.) ... (presumably that's not critical either really - no big deal if they temporarily end up in site-packages dir or whatever, out of sight of users).

@kinow
Copy link
Member Author

kinow commented Apr 24, 2019

@cylc/core - can we try to get this merged this week or next week? (depending on UK team availability late April I guess) ... it's been a long wait. It will make some other things easier, and details such as dependency version specificity can always be revisited later.

+1 this has become a blocker of issues here and in cylc/cylc-uiserver now.

@kinow is there anything much remaining to be resolved on this that you're aware of, from reviews so far? Do we need to move non-Python files (example suites as discussed above, and editor syntax files etc.) ... (presumably that's not critical either really - no big deal if they temporarily end up in site-packages dir or whatever, out of sight of users).

Nothing that cannot be revisited later IMO.

@hjoliver hjoliver mentioned this pull request Apr 24, 2019
Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

Looks good by 👀. I'll do some quick tests.

[bdist_rpm]
requires =
python3-colorama
python-isodatetime
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is something we need to address later?

Copy link
Contributor

@matthewrmshin matthewrmshin Apr 24, 2019

Choose a reason for hiding this comment

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

(Should be python3-isodatetime in the future.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this is something we need to address later?

Oh, well thought. Yup, or we could change that right now, but it hasn't changed in isodatetime yet I think. So I think later we can change that here once we change it there.

@matthewrmshin
Copy link
Contributor

I can get build, install, develop, sdist, bdist_wheel, etc to work.

Can't get the test command to work for some reason...

$ python3 setup.py test
usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
   or: setup.py --help [cmd1 cmd2 ...]
   or: setup.py --help-commands
   or: setup.py cmd --help

error: invalid command 'pytest'

@kinow
Copy link
Member Author

kinow commented Apr 25, 2019

Can't get the test command to work for some reason...

Depending on how you install the module, the test dependencies are not installed (default behaviour for setuptools/pip).

pip install ., for instance, finishes with:

Successfully installed colorama-0.4.1 cylc-8.0a0 ecdsa-0.13.2 future-0.17.1 isodatetime-1!2.0.0 jinja2-2.10.1 markupsafe-1.1.1 pyasn1-0.4.5 python-jose-3.0.1 pyzmq-18.0.1 rsa-4.0 six-1.12.0

The only way to achieve the test dependencies installed this way, is via another group in extras_require. We don't have a test or tests group yet, but we can create one (we can do that after this gets review/merged 🤞).

FWIW, here's the issue that was created (and rejected) for pip to install those dependencies as well: pypa/pip#1197 (comment)

I'm using pip install .[all] to get all the dependencies for cylc-flow. This group is common for development use, or for users that simply want all the dependencies of a project installed. The test dependencies are included here in this pull request.

After running pip install .[all] (which later would be pip install cylc[all] after uploading to PYPI), you should see

Successfully installed EmPy-3.3.4 Pygments-2.3.1 alabaster-0.7.12 atomicwrites-1.3.0 attrs-19.1.0 babel-2.6.0 certifi-2019.3.9 chardet-3.0.4 codecov-2.0.15 coverage-4.5.3 cylc-8.0a0 docutils-0.14 idna-2.8 imagesize-1.1.0 more-itertools-7.0.0 packaging-19.0 pluggy-0.9.0 py-1.8.0 pycodestyle-2.5.0 pyparsing-2.4.0 pytest-4.4.1 pytest-cov-2.6.1 pytz-2019.1 requests-2.21.0 snowballstemmer-1.2.1 sphinx-2.0.1 sphinxcontrib-applehelp-1.0.1 sphinxcontrib-devhelp-1.0.1 sphinxcontrib-htmlhelp-1.0.2 sphinxcontrib-jsmath-1.0.1 sphinxcontrib-qthelp-1.0.2 sphinxcontrib-serializinghtml-1.1.3 testfixtures-6.6.2 urllib3-1.24.2

But we are missing the dependency from setup_requires (yup, another key in setuptools for dependencies 😬). That dependency is installed only manually now... so you would have to run pip install pytest-runner. We can include that to our extra_requires later too 🤞 .

For what's worth, the setuptools documentation says:

(Note: projects listed in setup_requires will NOT be automatically installed on the system where the setup script is being run. They are simply downloaded to the ./.eggs directory if they’re not locally available already. If you want them to be installed, as well as being available when the setup script is run, you should add them to install_requires and setup_requires.)

Once that's installed, running tests should work fine. I believe Travis is passing as they should include pytest-runner in their distribution.

@matthewrmshin
Copy link
Contributor

(Just running pytest works.)

Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

Can use python3 setup.py with build_sphinx, sdist, bdist_wheel, install, etc. Pytest works on its own but not as part of python3 setup.py test. Installed cylc appears to work.

@hjoliver hjoliver assigned kinow and unassigned kinow Apr 25, 2019
@hjoliver
Copy link
Member

(I started testing this last night, will try to complete this morning.)

@hjoliver
Copy link
Member

INSTALL.md will need updating for this (but can do in a follow-up PR).

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Same results as Matt above - LGTM!

@kinow
Copy link
Member Author

kinow commented Apr 25, 2019

INSTALL.md will need updating for this (but can do in a follow-up PR).

That and also the user guide. After this PR I will work on - besides the ones for GraphQL, UI server, etc - more pull requests to address issues pointed by reviewers here and documentation.

@hjoliver
Copy link
Member

OK ... discussion with @oliver-sanders on dependency specificity still unresolved, but this ain't production code yet and we can tweak details later. So I'm going to merge this sucker!!! Thanks @kinow ... 🎉

@hjoliver hjoliver merged commit b1aa2f3 into cylc:master Apr 25, 2019
@hjoliver hjoliver mentioned this pull request Apr 26, 2019
@kinow kinow deleted the make-cylc-a-module-take-2 branch March 11, 2024 09:56
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.

Add setuptools and package Cylc as a Python module
5 participants