Skip to content

Tools#3

Merged
ScottSoren merged 10 commits into
masterfrom
tools
Jan 13, 2021
Merged

Tools#3
ScottSoren merged 10 commits into
masterfrom
tools

Conversation

@KennethNielsen

@KennethNielsen KennethNielsen commented Jan 9, 2021

Copy link
Copy Markdown

This PR introduces parts of my standard python development tool chain into ixdat for evaluation. The tools falls into two different categories, "tools" and something which I generally call "tool runners".

Of tools this introduces only pytestas a test runner and flake8 as a linter (takes care of some coding errors and coding style related stuff). flake8 is more or less standard if you want a fast and not too opinionated linter and I have over time come to prefer that over something like pylint. I did not include black to the configuration just yet, as I wanted to see if you are happy with it first.

Tool runners is likely the more interesting category, because those types of tools may not be too well known for scientists. These are tools to run your tools and perform tasks for you. You may ask, "why would I want a tool to run a tool?". The answer is that it can help you remember worthwhile options and in general make sure that it is run in an consistent manner and that tasks are performed easier and consistently.

As tools runners I have introduced invoke (http://www.pyinvoke.org/). The purpose of invoke is to easily make it possible and easy to do all the things/perform all the tasks, you need during development. Whenever you need to do something, you should check if invoke has a task for it and ask it to do it. If you perform a task that invoke hasn't been configured for and you do it more than twice, you should probably just go ahead and define an invoke task for it. In the olden days Unix developers would often use something like makefiles for this (https://en.wikipedia.org/wiki/Makefile), but they don't work on Windows which is the reason something like sphinx has both a makefile and a make bat file to compile the docs. Invoke aims to bridge that gap by providing as much of a platform independent command execution environment available as possible and by making its configuration file a pure python file, which means that you can yourself program your way around the whatever platform difference issues you may run into.

During development invoke commands to commonly use could be something like:

invoke lint

to run the linter

invoke tests

to run the tests

invoke checks

to run all the standard code checks (lint and test for now)

invoke clean

to delete all the temporary and cache files

invoke build-docs

to build the docs (not yet implemented)

to see a list of all available tasks do:

invoke --list

The second tool runner is tox (https://tox.readthedocs.io/en/latest/). tox is a tool to run all your QA tools in as consistent a manner as possible across as many of the supported python version as possible. It runs the QA tools in a virtual environment on a freshly installed version of your code. Because tox is meant mainly for continuous integration environments, it will try and perform all the tests on all the supported python versions. If they are unavailable, it will report an error, for this purpose I have configured an invoke task to run tox only on the python version that are available.

To run tox purely do:

tox

to run in parallel do:

tox -p auto

To run tox only on available python versions do

invoke tox

I tried to gather all the information that developers will needs about the tools in the TOOLS.rst file. Please give that a look.

NOTE: This is WIP since I know it has a directory conflict with #2, which I will resolve here when #2 is merged.

EDIT: I completely forgot to mention that the PR also includes a git hook, which is a mechanism to run tests and checks before pushing commits to a remote branch. Please see the details of it and installation instructions in TOOLS.rst.

@KennethNielsen KennethNielsen changed the title WIP Tools Tools Jan 11, 2021

@ScottSoren ScottSoren left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These tools look great!
I'll admit that I haven't got it running yet, but may as well get these comments to you anyway before I disappear for meetings the rest of the afternoon.
Most of the comments are questions which are more for my understanding than anything else but may as well get answers in the documentation.

My main concern about it is whether it adds a barrier of complexity that deters experimentalists who are not software developers from contributing. That is not a critique of the tools, but just a note that I think we have to make ixdat (or at least parts of ixdat) develop-able without the tools. It's more a governance thing, who's responsibility is it to do QA and when, and a luxury problem for a hypothesized time in the future when we have lots of contributers.

Comment thread TOOLS.rst Outdated
Comment thread TOOLS.rst Outdated
Comment thread TOOLS.rst
Comment thread TOOLS.rst Outdated
Comment thread setup.cfg
@@ -0,0 +1,8 @@
[flake8]
max-line-length = 95

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've been using 89, but okay. I think no matter what I use, I'll always often wish it was a few chars more.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

THAT is 100% and totally at the discretion of the project BDFL (https://en.wikipedia.org/wiki/Benevolent_dictator_for_life). Line length is something that people get religious about and I don't want to spend 2 seconds discussing it. I wrote 95 here, because in other projects we have been going with recommendations that have been mentioned in a few different places, which is aim for 90 but allow up to 95, which essentially means that the developer should break at 90 unless there is a good reason, like a long link or something like that, which can be finished in the last 5. But since linters aren't that intelligent, it needs to allow up to 95 if that is the policy. But I will put there whatever you say.

Comment thread tasks.py Outdated
Comment thread tasks.py Outdated
Comment thread tools/hooks/pre-push
Comment thread tox.ini Outdated
Comment thread TOOLS.rst Outdated

@ScottSoren ScottSoren left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, I tried to follow the instructions in tools.rst to get up and running, and find they are not complete, at least not for Windows with Anaconda Python and PyCharm.
There are comments with details.
I will keep playing around and update here if I get it working.

Comment thread TOOLS.rst Outdated

First we should locate the path of the tools. In case a separate
virtual environment is used, located e.g. in c:\venv\ixdat, then
c:\venv\ixdat\Scripts will be the path of tools. TODO anaconda.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm using anaconda and get the following, in the PyCharm Ipython console:

>>> import sys
>>> sys.executable
'C:\\ProgramData\\Anaconda3\\envs\\ixdat\\python.exe'

so...
/c/ProgramData/Anaconda3/envs/ixdat/python.exe
?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's not working for me. After adding the line to ~ /.bashrc and sourcing ~/.bashrc, it's there:

$ echo $PATH
/c/ProgramData/Anaconda3/envs/ixdat: ....

but invoke remains unrecognized by git.
I did remember to install invoke in ixdat's python environment in PyCharm (pip install invoke in Anaconda only gets it onto its (base) environment).
So I think some things are missing in these instructions.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah. I'm slowly learning that while anaconda is really good for getting a lot of high quality, high speed scientific code into the hands of scientists in a hurry I find that it is actually somewhat harder to figure out exactly what goes on. I actually intended to update this later specifically for anaconda (since I just uninstalled it in favor of an ordinary python installation and virtual environments, but since you show an interest in the hooks I think I will go back to get this to work and properly documentet.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Actually, can I persuade you do me one favor. Can I get you to run:

echo %path%

in your anaconda prompt and write the results here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

image

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Great! :|

I will see about re-installing anaconda shortly, but it will delay this PR a little further. I would prefer actually to merge it and fix it later as time allows (since it is marked with a TODO in TOOLS.rst) but I will leave that up to you.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That's fun. That command is mentioned like ... everywhere. Grumble grumble.

Comment thread TOOLS.rst
Also added a --single option to the tox invoke task and updated the
hooks files to use that option.
@KennethNielsen

Copy link
Copy Markdown
Author

Thanks for the review @ScottSoren

Ok, so I resolved most of the issues with a few pending feedback from you @ScottSoren. There are 2 things I wanted to comment in directly:

1
You mentioned a concern about the development tools creating to high a barrier for entry for new developers. I can assure you that I share that concern and I think it is important that we maintain one low barrier path for contribution (just because I like programming and sharing what I know, doesn't mean that I insist that everybody must learn;)). There is however not actually anything in this PR that conflicts with that. Using any of the regular tools or the tool runners are optional. As long as the git hook isn't enabled (which is an opt-in) there isn't anything that will interfere with the casual/scientific contributor.

What I suggest as the next step is to enable a continuous integration service, which will display a CI status (right besides the "merge-ability" status at the bottom of every PR, like so:
billede
(note, the status reduces to just a single line if all is green)

Now, I would suggest enabling the display of such status no-matter what, so that especially we, but also the interested new contributor, can see the CI status straight away (as a maintainer you have to reduce your manual chores as much as possible, trust me). However, whether an all green status is required for merge or not, is a decision. And we can set github up for the archive in a manner that reflects that decision. If you want to insist that all is green before merge, we can ask github to enforce that, but if you want to allow for merging "dirty" and then fixing the QA issues manually afterwards yourself (which is a policy I have seen some places), then we can just choose not to have github enforce it, in which case the CI status will be just for information.

2
The sharing of settings/command line between tox and invoke for QA tools. I wanted to mention here, that I'm willing to have that challenged. It is the first time I have done it like this, so I don't have prior experience to base the suggestion on. I have however been burned by not knowing with which settings a tool was run, which is the motivator for it, and in my view, avoiding multiple definitions of settings and commands is worth adding a little complexity. However, if you, even with the updates of comments I have now made, don't think it is worth it I will remove it. The duplication is limited at this point anyway.

@ScottSoren

ScottSoren commented Jan 13, 2021

Copy link
Copy Markdown
Member

Looks great!
Thanks for the additional documentation and in one case simplification. I want to re-iterate that I think these tools are fantastic, will definitely use them, and want it to be as easy as possible for new developers to use them as they get more into ixdat. I think that now they can find all the explanations they need in the code to enjoy that process.
I presently have tox and invoke running no problem on this branch in my Anaconda prompt, but do not yet have the commit hook working in my Windows PyCharm + Anaconda + git bash environment, but that is due to its quirks. I will make a new branch to document in .rst how to get it working in this particular setup. I also think we should merge this now!
Pending @kkrempl's read-through and approval.

@ScottSoren ScottSoren self-requested a review January 13, 2021 17:54
@ScottSoren ScottSoren merged commit 20ae552 into master Jan 13, 2021
@ScottSoren

ScottSoren commented Jan 13, 2021

Copy link
Copy Markdown
Member

Merged these tools! Thanks @KennethNielsen.
Some additions expected soon to tools.rst to cover e.g. Anaconda+PyCharm+git bash, as well as a "Getting Started Contributing" guide in the docs.

@ScottSoren ScottSoren deleted the tools branch January 13, 2021 21:16
@ScottSoren ScottSoren restored the tools branch January 13, 2021 21:21
@KennethNielsen KennethNielsen deleted the tools branch January 14, 2021 07:41
ScottSoren added a commit that referenced this pull request Feb 5, 2021
ScottSoren pushed a commit that referenced this pull request Mar 31, 2022
Revert "temperature-pressure MS plotting"
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.

3 participants