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

Cleaning up the RMG-Py GitHub with a robot #2474

Merged

Conversation

JacksonBurns
Copy link
Contributor

This PR is a followup to some discussion had at the previous RMG developer meeting, and is fodder for the meeting we have today.

The Problem

RMG-Py's GitHub page is... busy. As of writing this, we have 384 open issues and 56 outstanding PRs. Of course, many of these issues are no longer relevant (they have been resolved), and many of these PRs are severely out of date or abandoned.

The Proposed Solution

This PR proposes adding a bot which will:

  1. Mark any issue or PR that has received zero interaction in the past 90 days as stale.
  2. Automatically close any issue or PR which it has previously marked as stale if it does not receive any interaction within 30 days after being marked.

This bot is not a temporary measure - it will first help us resolve our significant backlog, but should also stay into the future.

The Reason

This is part of a larger effort to keep the GitHub issues and PRs doing what they are meant to do - tracking the software development of RMG. Right now they are being used at TODO lists and archives/discussion boards for scientific issues. We should limit the use of these pages for their intended purpose and move TODOs to project boards, internal documents, etc. and move important discussion to the Discussions tab.

As it stands, it is impossible for new developers to find an issue to work on or a PR to help out with because there are so many, and so many irrelevant ones. It's cloying for current developers as well (at least for me). I think this is an important move for the sustainability of RMG.

I would like to erassure everyone with outstanding PRs that none of the work will be lost, either. PRs and branches can be reopened at any point in the future.

Call for Input

I am aware that there are some PRs which do just take a really long time. My thinking here is that 90 days without any interactions at all (no commits, no comments), is likely either abandoned or severely out of date (and would likely need to be restarted anyway). I welcome feedback on the exact choice of timeline, but reiterate that even if this is seen as too aggressive of a window, no work is lost.

@JacksonBurns JacksonBurns added the Status: Ready for Review PR is complete and ready to be reviewed label Jun 21, 2023
@JacksonBurns JacksonBurns self-assigned this Jun 21, 2023
@rwest
Copy link
Member

rwest commented Jun 21, 2023

Regarding:

Right now they are being used at TODO lists and archives/discussion boards for scientific issues. We should limit the use of these pages for their intended purpose and move TODOs to project boards, internal documents, etc. and move important discussion to the Discussions tab

I caution against having too many places to put things because we then don't know where to look for them. Issues, wiki, kanban, slack, discussions, etc.
Having "institutional knowledge" be easily searchable is crucial.

At first I was worried about automatically closing things that aren't fixed. Sometimes they're real problems that we ought to address some day.
But because GitHub only have "open" and "closed", different types of closed, like "abandoned but not fixed" and "fixed" and "no longer relevant" have to be done by labels.
So if we're clear about which issues were actually fixed and which ones just timed out, I guess I'm ok with this.

Your vision is broadly that the only "open" issues are actively being worked on?

@JacksonBurns
Copy link
Contributor Author

I caution against having too many places to put things because we then don't know where to look for them. Issues, wiki, kanban, slack, discussions, etc. Having "institutional knowledge" be easily searchable is crucial.

I agree that we want to limit where things are spread out to - but I would argue that the current layout is not searchable. I believe that as long as we unambiguously document and stick to the idea that software problem = issue, science problem = discussion things would be both more searchable and more maintainable.

At first I was worried about automatically closing things that aren't fixed. Sometimes they're real problems that we ought to address some day. But because GitHub only have "open" and "closed", different types of closed, like "abandoned but not fixed" and "fixed" and "no longer relevant" have to be done by labels. So if we're clear about which issues were actually fixed and which ones just timed out, I guess I'm ok with this.

We can hash out the specifics on call, but there are 'exemptions' in the bot. We can make a label like bug that the bot will never close.

Your vision is broadly that the only "open" issues are actively being worked on?

I think that the open issues should be specific software bugs, enhancements, and features which are either currently being worked on and/or have some path forward. Scientific questions, or feature requests that are large in scope (and therefore have no specific way to implement with current RMG), belong in discussions.

@rwest
Copy link
Member

rwest commented Jun 21, 2023

Another approach worth looking at is Cantera where there's a separate repository for tracking "enhancements" https://github.com/Cantera/enhancements

But I forget exactly why we chose this over "discussions". It might have predated discussions, but it does have benefits of labels, and being able to close things.

One challenge we have with RMG is tracking issues with the database, with the website, and with RMG-Py itself. Often what starts as an "issue" with the observed chemistry could be a data and/or a software weakness and/or bug. It just starts as an issue.

@JacksonBurns
Copy link
Contributor Author

Another approach worth looking at is Cantera where there's a separate repository for tracking "enhancements" https://github.com/Cantera/enhancements

I've seen this in a number of orgs. It's not my favorite, since it makes it harder to tag/link feature requests with PRs, but I'm open to the idea.

But I forget exactly why we chose this over "discussions". It might have predated discussions, but it does have benefits of labels, and being able to close things.

Discussion are quite new, so that's probably the case.

One challenge we have with RMG is tracking issues with the database, with the website, and with RMG-Py itself. Often what starts as an "issue" with the observed chemistry could be a data and/or a software weakness and/or bug. It just starts as an issue.

This is true, and calls for a good triage system where maintainers can effectively respond to, move, and close issues from RMG-Py to wherever else they belong, be that a discussion on RMG-Py or on another repo. For effective triage, we first need to deal with the backlog.

@ChrisBNEU
Copy link
Contributor

I like this, but I think we will still need a few humans to sort through the issues/PRs marked as stale in the first sweep so they can be properly triaged. 30 days is a reasonable timeframe I think. Anything that is truly a bug can remain open and get assigned/reassigned, and stuff that is an enhancement/feature request/scientific discussion/etc. can be moved to the discussion board.

Getting everyone on the same page about what discussions should be used for is non-trivial (and which discussion board should be used, I can imagine getting issues that encompass rmgpy, database, and rms could be confusing). I haven't used them too much, but it looks like converting an issue to a discussion is pretty easy, not sure if the same can be said for prs though.

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #2474 (cbee876) into main (f2ae9d6) will decrease coverage by 0.04%.
The diff coverage is n/a.

❗ Current head cbee876 differs from pull request most recent head c17d4dd. Consider uploading reports for the commit c17d4dd to get more accurate results

@@            Coverage Diff             @@
##             main    #2474      +/-   ##
==========================================
- Coverage   48.27%   48.24%   -0.04%     
==========================================
  Files         110      110              
  Lines       30726    30726              
  Branches     8032     8032              
==========================================
- Hits        14834    14824      -10     
- Misses      14358    14370      +12     
+ Partials     1534     1532       -2     

see 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@JacksonBurns
Copy link
Contributor Author

Not related to this PR but this is the first PR where the regression tests have had a fair chance to run. The tests failed because one edge reaction was different. From the failed edge comparison:

Test model has 206 species.
Original model has 206 species.
Test model has 1508 reactions.
Original model has 1508 reactions.
The original model has 1 reactions that the tested model does not have.
rxn: CCCO[O](36) <=> [OH](21) + CCC=O(44)		origin: intra_H_migration
The tested model has 1 reactions that the original model does not have.
rxn: CCCO[O](34) <=> CC[CH]OO(45)		origin: intra_H_migration

Not sure if we expect this level of reproducibility.

@mjohnson541
Copy link
Contributor

Not related to this PR but this is the first PR where the regression tests have had a fair chance to run. The tests failed because one edge reaction was different. From the failed edge comparison:

That result suggests a bug to me...one that may be quite complicated, but probably shouldn't be discussed here in this PR.

@mjohnson541
Copy link
Contributor

I think a lot of this discussion is predicated on the assumption that we have natural separation between scientific issues and software issues...in many cases I think the two issues intertwine and there isn't a clear distinction...and in even more cases it isn't obvious which kind of issue it is at the outset. In fact the choice may depend significantly on what the reader needs/expects from RMG.

What does enforcing this binary distinction provide us with that's so valuable here?

@JacksonBurns
Copy link
Contributor Author

I think a lot of this discussion is predicated on the assumption that we have natural separation between scientific issues and software issues...in many cases I think the two issues intertwine and there isn't a clear distinction...and in even more cases it isn't obvious which kind of issue it is at the outset. In fact the choice may depend significantly on what the reader needs/expects from RMG.

What does enforcing this binary distinction provide us with that's so valuable here?

I think this provides a strategy for figuring out if a 'problem' is a software issue or scientific issue, and by categorizing them we can more effectively work on them.

Throughout our docs, and in the RMG training, we tell people that "if you need help, open an issue on GitHub" and I think that's great. We have a single, definitive place for people to walk up with 'problems'.

Right now, problems that are more like "how can I model this system" or "why does my mechanism look weird" are sitting in the same place as "i wish I could use this approximation" or "this error popped up when I did this". The latter two belong on the issues page because we could implement them in software - for the former two, maybe the answer is a software fix or a scientific discussion, and we can hash that out first on the issues page, leave it there if relevant or move it if not.

When the two are clearly separated, it will be easier to know what to work on (1) for current developers and (2) for people that are just starting/interested in helping out.

@mjohnson541
Copy link
Contributor

I guess maybe I really see this as a carriage without a horse. Getting ontop of and organizing all of RMG's issues is good, but it's going to take a significant amount of labor. A significant portion of issues may require an hour or two just in order to make the classification you need let alone solve them.

I don't think this system works well at all if we aren't on top of the issues and PR...if someone puts up an issue or PR and no one picks it up do we really want to send them a reminder every 3 months that we ignored their issue/PR and we're interested in closing it? I think this requires a certain amount of commitment. I've managed a development team where we had rotation system. We could have a system where someone is made responsible to respond or get the right person to help respond to whenever a new issue pops up, but I feel like that kind of buy in important before shifting to something that I think really needs that kind of commitment like this.

Here are very recent issues that were created, but no developer responded: #2371, #2370, #2367, #2391, #2402

I don't really buy the idea that new developers are just going to hop in and take on RMG issues without needing to talk to a developer anyway. Ideally you would have them working on an issue that is easy to solve and understand, but teaches them about the code...but I'm not sure how many of those happy medium issues we really usually have.

@JacksonBurns
Copy link
Contributor Author

JacksonBurns commented Jun 21, 2023

Note from dev meeting - add an exemption label 'bug' and label closed as stale specifically.

@JacksonBurns
Copy link
Contributor Author

I guess maybe I really see this as a carriage without a horse. Getting ontop of and organizing all of RMG's issues is good, but it's going to take a significant amount of labor. A significant portion of issues may require an hour or two just in order to make the classification you need let alone solve them.

I don't think this system works well at all if we aren't on top of the issues and PR...if someone puts up an issue or PR and no one picks it up do we really want to send them a reminder every 3 months that we ignored their issue/PR and we're interested in closing it? I think this requires a certain amount of commitment. I've managed a development team where we had rotation system. We could have a system where someone is made responsible to respond or get the right person to help respond to whenever a new issue pops up, but I feel like that kind of buy in important before shifting to something that I think really needs that kind of commitment like this.

I don't agree that this will be more labor - we still have to undergo all same steps of deciding who works on which issues, finding out who is qualified to work on them, implementing, testing, etc. The benefit here would be that we do that on maybe tens of issues instead of hundreds, and there will be a more obvious way to do it.

As far as the perception from users -- I think us putting anything on the PR/issue is better than just leaving it with no interaction indefinitely. Ideally, if we can knock RMG down to a few tens of issues, we won't have to worry about non-interactions anyway.

Here are very recent issues that were created, but no developer responded: #2371, #2370, #2367, #2391, #2402

I see this as symptomatic of the problem rather than evidence to not implement this change.

I don't really buy the idea that new developers are just going to hop in and take on RMG issues without needing to talk to a developer anyway. Ideally you would have them working on an issue that is easy to solve and understand, but teaches them about the code...but I'm not sure how many of those happy medium issues we really usually have.

I should rephrase - the goal is not to have perfectly separated issues vs discussions, where the former is a matter of solely programming something and the latter is a matter of thinking, to then be followed by an issue. The goal is to give people who want to learn how to develop RMG a starting point where think they could start programming instead of taking a class. They may later realize they potentially need input from developers/experts - and with fewer issues that is something we could actually hope to deal with.

@mjohnson541
Copy link
Contributor

Everyone else seems broadly okay with this and I'm not trying to say I think this is disastrous or something. So don't let me stop this or anything.

I think the most important thing I have to say is that how got here, in terms of issues at least, is not because we didn't have this system, it's because we were unable to ever sustain developer commitment to take responsibility for all issues from open to close. That is the change is really required to fundamentally fix this. All the system really does by itself to help that component is automatically close a bunch of really old issues so the list of issues is smaller, which is nice, but as demonstrated the developer team currently is not even touching many very recent issues. New issues can be discussed in developer meetings as mentioned in the meeting, which is a good start, but we need to be committed to doing that every week and we need people to commit every week to putting in the actual work time to analyze and resolve the more complex issues. I think that's what will really result in lasting change as far as issue handling, but it can be hard to do that with a limited number of developers that have lots of other commitments.

@rwest
Copy link
Member

rwest commented Jun 21, 2023

I agree that this system won't solve the problem of people not fixing old issues. But it might solve the problem of people not closing abandoned issues. I think the consensus after today's meeting is that we should give it a go. The action in this PR looks consistent with what we've discussed and, as quickly as I read it, the action's instructions.

@rwest rwest merged commit c1c6034 into ReactionMechanismGenerator:main Jun 21, 2023
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Ready for Review PR is complete and ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants