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

Kill wreck and other unused code #1988

Merged
merged 22 commits into from
Feb 13, 2019
Merged

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Feb 3, 2019

This is an initial pass at euthanizing the old execution system as requested in #1984. Continuing along those lines, other code and services of which wreck was the main or only user is removed. Small modifications were made in other places to get even more old code amputated, including, finally, the removal of libcompat.

Still todo:

  • revisit flux-aggregate.c. This was a hack and slash job and now that I think of it should be under t if it is kept at all
  • consider removing modules/cron -- there isn't a use case for this module anymore (actually ever since hierarchical lwj directory), and flux-cron is the last utility written in lua.
  • redo some lua test scripts under t/scripts in shell or C and remove the rest of the Lua bindings.

Then we can move on to removal of older kvs watch and other kvs_classic calls etc.

There will need to be similar work done in flux-sched before this get merged. If we had thought ahead we could have tied the current flux-sched to v0.11.0 tag of flux-core, but since we've applied changes to flux-sched that require newer flux-core we'll instead have to update flux-sched first.

@garlick
Copy link
Member

garlick commented Feb 4, 2019

Yikes, I didn't expect the void left behind by this PR to be so terrifying.

@grondo
Copy link
Contributor Author

grondo commented Feb 4, 2019

Its like deleting the last 5 years of my life. Kind of liberating!

@garlick
Copy link
Member

garlick commented Feb 4, 2019

Maybe I should have shot old Yeller instead of making you do it :-/

@grondo
Copy link
Contributor Author

grondo commented Feb 4, 2019

Ok, kvs lua bindings are removed. Now I think the only user of kvs_classic is bindings/python/flux/kvs.py.

@garlick
Copy link
Member

garlick commented Feb 4, 2019

Restarted a builder that failed on #1986

@chu11
Copy link
Member

chu11 commented Feb 4, 2019

Ok, kvs lua bindings are removed. Now I think the only user of kvs_classic is bindings/python/flux/kvs.py.

I thought I un-classiced kvs.py in #1748. But the old watch code has been deprecated since then, so perhaps we gotta update that part too. That said, I don't see any python using the watch code, so perhaps that code can be yoinked? (But also yet again, #1649 says we gotta re-work tons of that python lib)

@grondo
Copy link
Contributor Author

grondo commented Feb 4, 2019

That said, I don't see any python using the watch code, so perhaps that code can be yoinked?

Yeah, my hesitation is that we do have users using the python bindings, so I don't know what is safe to remove. However, the point of this PR is removal of older interfaces regardless of current usage, so I'll just go ahead and remove that code along with the old kvs_watch interfaces if that is ok by you.

@grondo
Copy link
Contributor Author

grondo commented Feb 4, 2019

I'll just go ahead and remove that code along with the old kvs_watch interfaces

A couple kvs tests still use watch: t/kvs/watch.c looks like it should just go away, but t/kvs/transactionmerge.c probably will have to be updated to flux_kvs_lookup?

@chu11
Copy link
Member

chu11 commented Feb 4, 2019

but t/kvs/transactionmerge.c probably will have to be updated to flux_kvs_lookup?

There are probably a few that need to be updated. Would you like me to do that as a PR before this PR is merged? Or shall we just leave all watch code as is and do it separately?

@grondo
Copy link
Contributor Author

grondo commented Feb 4, 2019

Or shall we just leave all watch code as is and do it separately?

Yeah, this seems like the best approach. I thought there were just a couple uses of watch that could be easily converted, but it is a bit more complex than that and probably not valid for this PR.

@grondo grondo force-pushed the kill-wreck branch 3 times, most recently from 3c586c4 to b2f64ab Compare February 4, 2019 22:42
@grondo
Copy link
Contributor Author

grondo commented Feb 4, 2019

Ok, I've pushed some further code removal and other cleanup (removed some accidental debug code from src/cmd/builtin/hwloc.)

At this point there are many unused functions in kvs_classic.c (flux_kvs_get, flux_kvs_put, flux_kvs_unlink, flux_kvsdir_get, flux_kvs_commit_anon) , but rather than remove them now, we'll just wait for a future kvs API cleanup PR.

To fully remove the Lua bindings, we'd have to remove the cron module or rewrite the frontend in python/C, and find alternatives for the test helper scripts waitfile.lua and event-trace*.lua. That seemed a bit too much to take on here, so I've stopped a bit short. My apologies.

@grondo
Copy link
Contributor Author

grondo commented Feb 4, 2019

Oh, I also had thought about moving flux-aggregate off under t/. However, that would be a bit disruptive to this PR at this point (would require extensive rebasing). If people feel strongly about it I could still do that work here. (too bad aggregate support was added to flux-hwloc or the module could have just been removed.. hmm.)

@garlick
Copy link
Member

garlick commented Feb 5, 2019

Oh, I also had thought about moving flux-aggregate off under t/. However, that would be a bit disruptive to this PR at this point (would require extensive rebasing). If people feel strongly about it I could still do that work here. (too bad aggregate support was added to flux-hwloc or the module could have just been removed.. hmm.)

I think this PR already cuts pretty deep. My $0.02 would be to leave working bits unrelated to wreck alone for now, with the exception of the deprecated stuff you've already culled.

Won't the aggregator module be useful for collecting exit codes and similar reductions? If it has some deficiency, maybe we should get an issue open on it?

Similarly, the lua bindings have provided a lot of utility thus far. Maybe one could make a case that we should go all in with python as our scripting language, but I'm sure there are pros and cons to that choice and it seems like a discussion to have in an issue.

I think cron is potentially useful to workflow implementors that may want to periodically dump stats or other activities.

@grondo
Copy link
Contributor Author

grondo commented Feb 5, 2019

I've updated some of the commit messages to close open issues and force-pushed.

@grondo
Copy link
Contributor Author

grondo commented Feb 5, 2019

rebased on current master

@autorebase autorebase bot deleted the kill-wreck branch February 8, 2019 00:15
@grondo grondo restored the kill-wreck branch February 8, 2019 00:16
@grondo
Copy link
Contributor Author

grondo commented Feb 8, 2019

Oops, I was testing autorebase and used a real branch.

Well, rebased (by a tiny panda) on current master.

@autorebase autorebase bot deleted the kill-wreck branch February 8, 2019 01:29
@codecov-io
Copy link

Codecov Report

Merging #1988 into master will increase coverage by 0.28%.
The diff coverage is 82.28%.

@@            Coverage Diff             @@
##           master    #1988      +/-   ##
==========================================
+ Coverage   80.13%   80.42%   +0.28%     
==========================================
  Files         195      180      -15     
  Lines       35191    29394    -5797     
==========================================
- Hits        28201    23640    -4561     
+ Misses       6990     5754    -1236
Impacted Files Coverage Δ
src/common/libflux/conf.c 100% <ø> (ø) ⬆️
src/modules/cron/cron.c 79.9% <ø> (+1.17%) ⬆️
src/broker/broker.c 77.47% <ø> (+0.06%) ⬆️
src/bindings/lua/lutil.c 89.28% <ø> (+11.16%) ⬆️
src/cmd/flux.c 84.04% <ø> (-0.17%) ⬇️
src/common/libaggregate/aggregate.c 86.81% <100%> (+5.91%) ⬆️
src/cmd/builtin/hwloc.c 80.63% <100%> (+0.09%) ⬆️
src/cmd/flux-aggregate.c 77.77% <77.77%> (ø)
src/bindings/lua/flux-lua.c 87.39% <85.48%> (+5.07%) ⬆️
src/common/libpmi/pmi_strerror.c 0% <0%> (-66.67%) ⬇️
... and 14 more

@garlick garlick self-requested a review February 13, 2019 19:43
Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

I'm good with this going in ASAP.

@garlick
Copy link
Member

garlick commented Feb 13, 2019

@grondo, feel free to add merge-when-passing if you think enough people have given the thumbs up here. I would do it but I didn't want to rush it if you felt more review was required.

@grondo
Copy link
Contributor Author

grondo commented Feb 13, 2019

I think I'll consider double-hooray on the initial post as enough acks. Let's get this over with.
(I hope auto-close of issues works even when the bot merges...)

@mergify mergify bot merged commit 16c1e9d into flux-framework:master Feb 13, 2019
@grondo
Copy link
Contributor Author

grondo commented Feb 13, 2019

Yeah, this is a big drawback of using the bot -- it doesn't seem to auto-close issues 😠

@grondo
Copy link
Contributor Author

grondo commented Feb 13, 2019

ah, crap. No it seems I may have pushed from an older branch somehow. Wow, that's too bad.

I think the main differences were just the commit messages, but I'll double check to be sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants