Remove cylc.profiling package and cylc-profile-battery

[kinow]

For #2991 (once this one gets merged, will test and close #2991 if no other command failed in my Anaconda environment).

The cylc-profile-battery command tries to locate the directory of cylc script, and locate the base Cylc installation directory. Then it fails if that directory is not a Git repository (i.e. does not have a .git folder).

This causes issues when running the tests with Cylc being installed via pip under a virtual environment in Anaconda.

In #2997 we changed the way the Cylc version is located, so that there is no more need to have a git directory. And now in this pull request we are using that approach instead of using the git command line.

And instead of trying to locate the Cylc directory, the script will default to the old behaviour of assuming . is a Cylc installation of .git, but also accept an optional parameter that defines where is the Cylc directory we would like to use in the experiments.

As per discussion (see comments below) this PR removes the cylc.profiling package, the cylc-profile'-battery and updates the documentation.

[kinow]

First executed cylc profile-battery -e hello-world and got the experiment running successfully.

...
$ rm -rf /home/kinow/cylc-run/profile-15524351935191412

Version  Run  Elapsed Time (s)  CPU Time - Total (s)  Max Memory (kb)  
HEAD     run  2.97              0.6736363636363637    35721.09090909091

Results for experiment "hello-world" have been written out to "/home/kinow/Development/python/workspace/cylc/.profiling/plots/hello-world-1552435229"

And after the change it works too after removing my .profiling directory:

...
$ rm -rf /home/kinow/cylc-run/profile-15524366025308645

Version  Run  Elapsed Time (s)    CPU Time - Total (s)  Max Memory (kb)
HEAD     run  2.9518181818181817  0.6736363636363635    35728.0        

Results for experiment "hello-world" have been written out to "/home/kinow/Development/python/workspace/cylc/.profiling/plots/hello-world-1552436637"

Then I did the following test.

• kinow@/home/kinow: rm -rf .profiling
• kinow@/home/kinow: mv .git .git2
• kinow@/home/kinow: cd /tmp && git clone git clone https://github.com/cylc/cylc.git && cd -
• cylc profile-battery -e hello-world: Fails with ERROR: profiling requires cylc to be a git repository.
• cylc profile-battery -e hello-world /tmp/cylc

The experiment is executed successfully, and the .profiling folder was correctly created under /tmp/cylc/.profiling/:

...
$ rm -rf /home/kinow/cylc-run/profile-15524424398637655

Version  Run  Elapsed Time (s)  CPU Time - Total (s)  Max Memory (kb)   
HEAD     run  2.89090909090909  0.6309090909090909    35690.545454545456

Results for experiment "hello-world" have been written out to "/tmp/cylc/.profiling/plots/hello-world-1552442474"

[kinow]

Added a couple TODO markers, as this current approach requires to change a constant in the cylc.profiling package - even though constants should be immutable.

To remove these TODO markers, I reckon we would have to modify the current design of the cylc.profiling package, which would take longer. Hopefully this current fix is sufficient to proceed with this issue and #2990 (blocked by this one ATM).

Travis-CI passed in my fork, but I had to kick it three times. Will kick it here too during the day if required 👍

[hjoliver]

@kinow - I haven't looked at your code yet, but for use with installed versions (not git clones) we should probably assume our recommended side-by-side installation structure (e.g. /usr/local/bin/cylc uses $CYLC_VERSION to find /opt/cylc/$CYLC_VERSION/) ... possibly overridden by optional explicit locations as a quick read above suggests you've done.

[hjoliver]

Or (sorry - not thinking!) are we expecting use of cylc profile-battery only with git clones?

[kinow]

Or (sorry - not thinking!) are we expecting use of cylc profile-battery only with git clones?

I think this is a requirement for cylc profile-battery. You must provide a folder with a clone of Cylc, so that it is able to either use the current version, or run the experiment against multiple versions and report the benchmarking results (that's my current high-level understanding of that package/tool).

[kinow]

When users install cylc in the future, possibly via pip install cylc... they will still get cylc profile-battery. But installing via pip, there won't be a .git repository anywhere.

So users will now be required to clone the repository somewhere, in order to use this tool. Not sure if the best approach, but at least this way cylc profile-battery can still be used even after Anaconda, pip, setup.py, etc.... (and I can continue testing our current work on the setup.py branch 😁 ).

[hjoliver]

Makes sense; let's see what @oliver-sanders thinks.

[oliver-sanders]

So users will now be required to clone the repository somewhere

Yes. The purpose of the cylc profile-battery is to allow developers to profile their development branches. It is only expected to work on a development clone. I can't think why we would want it to work on an installation via pip/conda.

[hjoliver]

I can't think why we would want it to work on an installation via pip/conda.

I can imagine that site admins who install Cylc in critical production environments might want to check that performance has not degraded with the next release (possibly with a local suite).

We don't have to support that, of course (and maybe we shouldn't) ... but I imagine it would be even easier to compare to installed releases than two git versions checked out on the fly.

[hjoliver]

If we don't want to support that, can we have cylc profile-battery removed from releases?

[kinow]

If we don't want to support that, can we have cylc profile-battery removed from releases?

Maybe we could promote it to a top level project? cylc/profile-battery or cylc/cylc-profile-battery. It could use cylc as a dependency, and be a standalone project.

Site admins could then manage where to install that, how to use, etc.

[hjoliver]

I quite like that idea.

[oliver-sanders]

I quite like that idea.

Ditto!

Though we would need to be slightly careful not to break it.

The cylc profile-battery could really do with a re-write (part of the reason it's so horrible is that we needed lots of back-compatibility for a report). I got part way through and had to bail to get other work done.

[kinow]

So what about this plan of action?

1. Use this fix so that the work on the setup.py can continue for now
2. I will create a separate ticket to move cylc-profile-battery to its own repository
3. I will mark that ticket as BLOCKED by the setup.py ticket (i.e. work on the cylc-profile-battery depends first on being able to use something like from cylc import __version__ ...)

[kinow]

And I really liked the code of the profile battery. But if you think it could be improved, then I really want to see the new design and play with it! I suspect having it in its own repository will make it actually easier to re-write it.

[hjoliver]

@kinow 's plan of action looks good to me. I'll defer to @oliver-sanders as the code owner though. (And perhaps @oliver-sanders can also check that his UK colleagues don't object).

[oliver-sanders]

Possible alternative:

1. Delete cylc profile-battery.
2. Raise an issue to move it.
3. Lets get going on packaging!!!

We've already deleted cylc review with plans to re-incarnate in the near future so that wouldn't be too bad.

[kinow]

Oh, that would be even easier! If others are OK with that too, I will update (or close an raise a new one) the pull request. Cheers!

[matthewrmshin]

@kinow please go ahead.

[kinow]

Pull request update to now remove the cylc.profiling package, the cylc-profile-battery, and update documentation.

All tests passing, and my ol' suite number five ran without issues locally.

Ready for review again.

Cheers
Bruno

[hjoliver]

(Now rather easy to review!)

[matthewrmshin]

This is now in conflict now that the typos branch is merged.

[kinow]

This is now in conflict now that the typos branch is merged.

Thanks @matthewrmshin ! I avoided touching cylc-profile-battery and another file because I thought it would cause conflicts, but forgot about etc/cylc-bash-completion.

Conflict undone!

[oliver-sanders]

I think the profiling suites should stay with the profile battery. In fact I think all the dev suites are performance examples so we could probably move them all?

[kinow]

In fact I think all the dev suites are performance examples so we could probably move them all?

Oh, I had no idea. Just pushed another commit that removes etc/dev-suites. If those suites were not supposed to be removed, let me know and I will drop the commit 👍 Thanks!!!

[oliver-sanders]

I just spotted there are some xtrigger suites in there, not too sure why they are in dev-suites, should they be moved to examples @hjoliver?

[hjoliver]

1. etc/dev-suites/ is a historical dumping ground for suites used for manually testing or illustrating some kind of performance issue
2. it also holds suites used as built-in cases for cylc profile-battery (some of these originate from 1.)
3. the xtrigger examples should be moved to /etc/examples/, I think I just forgot to do that when finishing up the xtrigger development last year.

[hjoliver]

Just wondering about the logic of moving profiling to a new repository vs leaving the test battery in the main repository.

I guess that makes sense because the test battery is to check that the current source version passes all tests, but the profiler compares two arbitrary cylc versions - i.e. it is not inherently tied to the current source version.

[hjoliver]

In principle the test battery could also be used in a release, after installation, to check that everything works.

Any other developer-specific files - these days just a few files under etc/ should probably be removed from main repo.

[kinow]

Good point @hjoliver , I hadn't considered the test-battery. I think it makes sense to move the profile-battery as it can be maintained as a separate project, with its own release cycle, bugs/issues/pull requests/features.

And the profile-battery should not change if we change Cylc's internal API. So there is a more clear separation between Cylc and profile-battery. It could be possible that it would still work for Cylc 9 or Cylc 10, maybe with some minor adjustments.

But the test-battery relies in some parts on API in Cylc that is not exposed to public. Some tests verify - as far as I could tell - if parsing is working, task pool state, messages, etc. So this one is much more tangled with Cylc.

Changing features in Cylc require changing test-battery some times. But not the profile-battery I think (maybe create a new experiment?).

[hjoliver]

Yes, that's right. So we're doing the right thing - profiler out, test battery in.

[kinow]

the xtrigger examples should be moved to /etc/examples/, I think I just forgot to do that when finishing up the xtrigger development last year.

Done in 8f2b749! 🎉

[oliver-sanders]

Well this is a nice easy review. Excellent code style, every line added is beautiful.