Re: MR template text

[Julian Leviston]

I gave it a shot at your behest, but I find your writing far more eloquent than 
mine, so it could no doubt deal with some editing.


This is a work in progress Merge Request!
 
If you want (a) to get code reviews from others, or
(b) to land the patch in GHC,
please do follow these guidelines.

* [ ] (you can do this last) please replace this entire notice and checklist 
template with the following:
  - a description of what the Merge Request does. For single-commit MRs, the 
commit message is often perfect
  - A reference (e.g. #19415) to the ticket that led to this MR, and that 
describes the
  problem that this MR solves.  Almost all MRs need a ticket, except the tiniest
  changes (e.g. code formatting)
- A ticket describes a *problem*
- A merge request describes a *solution* to that problem.
* [ ] commits need to be either individually buildable or squashed
* [ ] commits need to have commit messages which describe *what they do*
   (referring to [Notes][notes] and tickets using `#` syntax when
   appropriate)
* [ ] add source comments describing your change. For larger changes you
   likely should add a [Note][notes] and cross-reference it from the relevant
   places.
* [ ] add a [testcase to the
   
testsuite](https://gitlab.haskell.org/ghc/ghc/wikis/building/running-tests/adding).
 
If you have any questions don't hesitate to open your merge request and inquire
in a comment. If your patch isn't quite done yet please do add a `WIP:` prefix 
to
your MR title.
 
[notes]: For general style guidance and information on notes see
https://gitlab.haskell.org/ghc/ghc/wikis/commentary/coding-style



> On 24 Feb 2021, at 9:16 am, Simon Peyton Jones  wrote:
> 
> Thanks Julian
>  
> I am by definition the wrong person to judge (or even write) text like this. 
>  
> Could you possibly have a go at editing the draft I sent so that you think it 
> has the right tone and content?  The current one is not working well.  Your 
> draft will almost certainly be better than mine.
>  
> Simon
>  
> From: Julian Leviston  
> Sent: 23 February 2021 21:59
> To: Simon Peyton Jones 
> Cc: ghc-devs 
> Subject: Re: MR template text
>  
>  
> Hi Simon, list et al,
>  
> I’ve only contributed a couple of times, but I personally found the checklist 
> invaluable to guide me (and remind me of) what needed to be done in total. In 
> addition, giving folks a checklist that they can actually check off gives us 
> a common set of agreed upon things that’s needed in an MR right in the MR, 
> which is nice to folks.
>  
> I wonder if we could reword it to say it’s still a work in progress or words 
> to that effect at the top, and make the system not allow MRs to be built 
> and/or merged unless they edit that text away, as well as have a bot inform 
> them of why this is? :) I like the idea of the system guiding us through the 
> process.
>  
> Regards,
> Julian
>  
> Would it be possible to get our tooling (a bot?) to nudge us if we haven’t 
> changed it?
>  
> On 24 Feb 2021, at 3:14 am, Simon Peyton Jones via ghc-devs 
> mailto:ghc-devs@haskell.org>> wrote:
>  
> I often see MRs in my inbox that say
> 
> Thank you for your contribution to GHC!
> Please take a few moments to verify that your commits fulfill the following:
> [ ] are either individually buildable or squashed
>  
> 
> This is because the author hasn’t changed the Description of the MR, but 
> rather has left the template text unchanged.
> 
> As a way to “nudge” authors to give reviewers more information, I suggest 
> replacing the template text with the draft below.  Does anyone have any 
> views, for or against?
> 
> Simon
> 

___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

RE: MR template text

[Simon Peyton Jones via ghc-devs]

Thanks Julian

I am by definition the wrong person to judge (or even write) text like this.

Could you possibly have a go at editing the draft I sent so that you think it 
has the right tone and content?  The current one is not working well.  Your 
draft will almost certainly be better than mine.

Simon

From: Julian Leviston 
Sent: 23 February 2021 21:59
To: Simon Peyton Jones 
Cc: ghc-devs 
Subject: Re: MR template text


Hi Simon, list et al,

I’ve only contributed a couple of times, but I personally found the checklist 
invaluable to guide me (and remind me of) what needed to be done in total. In 
addition, giving folks a checklist that they can actually check off gives us a 
common set of agreed upon things that’s needed in an MR right in the MR, which 
is nice to folks.

I wonder if we could reword it to say it’s still a work in progress or words to 
that effect at the top, and make the system not allow MRs to be built and/or 
merged unless they edit that text away, as well as have a bot inform them of 
why this is? :) I like the idea of the system guiding us through the process.

Regards,
Julian

Would it be possible to get our tooling (a bot?) to nudge us if we haven’t 
changed it?

On 24 Feb 2021, at 3:14 am, Simon Peyton Jones via ghc-devs 
mailto:ghc-devs@haskell.org>> wrote:

I often see MRs in my inbox that say
Thank you for your contribution to GHC!
Please take a few moments to verify that your commits fulfill the following:
[ ] are either individually buildable or squashed

This is because the author hasn’t changed the Description of the MR, but rather 
has left the template text unchanged.
As a way to “nudge” authors to give reviewers more information, I suggest 
replacing the template text with the draft below.  Does anyone have any views, 
for or against?
Simon

___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: MR template text

[Julian Leviston]

Hi Simon, list et al,

I’ve only contributed a couple of times, but I personally found the checklist 
invaluable to guide me (and remind me of) what needed to be done in total. In 
addition, giving folks a checklist that they can actually check off gives us a 
common set of agreed upon things that’s needed in an MR right in the MR, which 
is nice to folks.

I wonder if we could reword it to say it’s still a work in progress or words to 
that effect at the top, and make the system not allow MRs to be built and/or 
merged unless they edit that text away, as well as have a bot inform them of 
why this is? :) I like the idea of the system guiding us through the process.

Regards,
Julian

Would it be possible to get our tooling (a bot?) to nudge us if we haven’t 
changed it?

> On 24 Feb 2021, at 3:14 am, Simon Peyton Jones via ghc-devs 
>  wrote:
> 
> I often see MRs in my inbox that say
> 
> Thank you for your contribution to GHC!
> Please take a few moments to verify that your commits fulfill the following:
> [ ] are either individually buildable or squashed
>  
> 
> This is because the author hasn’t changed the Description of the MR, but 
> rather has left the template text unchanged.
> 
> As a way to “nudge” authors to give reviewers more information, I suggest 
> replacing the template text with the draft below.  Does anyone have any 
> views, for or against?
> 
> Simon
> 

___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: MR template text

[Alan & Kim Zimmerman]

I'm not sure if it would work here, but I have seen some issue templates,
e.g. for haskell-language-server on github[1], use comment markup in the
template.

e.g.



This is not rendered on the web or email views, but is seen when editing.

Alan

[1] https://github.com/haskell/haskell-language-server/issues/new


On Tue, 23 Feb 2021 at 21:21, Richard Eisenberg  wrote:

> I'm happy with this direction, though I don't think we should expect that
> even capital letters will actually make people notice the text. One small
> change to suggest, below:
>
> On Feb 23, 2021, at 11:14 AM, Simon Peyton Jones via ghc-devs <
> ghc-devs@haskell.org> wrote:
>
> *Proposed new template*
> PLEASE REPLACE ALL OF THIS TEXT with a description of your merge request,
> including
>
> * A description of what the Merge Request does.  For a single-commit MR, a
> copy of the
>   commit message is often perfect.
>
> * A reference (e.g. #19415) to the ticket that led to this MR, and that
> describes the
>   problem that this MR solves.  Almost all MRs need a ticket, except the
> tiniest
>   changes (e.g. code formatting)
>   - A ticket describes a *problem*
>   - A merge request describes a *solution* to that problem.
>
> While you are encouraged to write a good MR Description, it’s not
> compulsory.
> You could just be putting up the MR to share with a colleague, for example.
>
> But if you want (a) to get code reviews from others, or
> (b) to land the patch in GHC,
> please do follow these guidelines.
>
>
> If you are not ready for wide review, please start the MR name with the
> prefix "WIP:", for "work in progress".
>
>
> For general style guidance see
> https://gitlab.haskell.org/ghc/ghc/wikis/commentary/coding-style
>
>
>
> Richard
> ___
> ghc-devs mailing list
> ghc-devs@haskell.org
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: MR template text

[Richard Eisenberg]

I'm happy with this direction, though I don't think we should expect that even 
capital letters will actually make people notice the text. One small change to 
suggest, below:

> On Feb 23, 2021, at 11:14 AM, Simon Peyton Jones via ghc-devs 
>  wrote:
> 
> Proposed new template
> 
> PLEASE REPLACE ALL OF THIS TEXT with a description of your merge request, 
> including
>  
> * A description of what the Merge Request does.  For a single-commit MR, a 
> copy of the
>   commit message is often perfect.
>  
> * A reference (e.g. #19415) to the ticket that led to this MR, and that 
> describes the
>   problem that this MR solves.  Almost all MRs need a ticket, except the 
> tiniest
>   changes (e.g. code formatting)
>   - A ticket describes a *problem*
>   - A merge request describes a *solution* to that problem.
>  
> While you are encouraged to write a good MR Description, it’s not compulsory.
> You could just be putting up the MR to share with a colleague, for example.
>  
> But if you want (a) to get code reviews from others, or 
> (b) to land the patch in GHC, 
> please do follow these guidelines.

If you are not ready for wide review, please start the MR name with the prefix 
"WIP:", for "work in progress".

>  
> For general style guidance see
> https://gitlab.haskell.org/ghc/ghc/wikis/commentary/coding-style 
> 


Richard___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: Changes to performance testing?

[Ben Gamari]

Ben Gamari  writes:

> Hi all,
>
> Recently our performance tests have been causing quite some pain. One
> reason for this is due to our new Darwin runners (see #19025), which
> (surprisingly) differ significantly in their performance characteristics
> (perhaps due to running Big Sur or using native tools provided by nix?).
>
> However, this is further exacerbated by the fact that there are quite a
> few people working on compiler performance currently (horray!). This
> leads to the following failure mode during Marge jobs:
>
>  1. Merge request A improves test T1234 by 0.5%, which is within the
> test's acceptance window and therefore CI passes
>
>  2. Merge request B *also* improves test T1234 by another 0.5%, which
> similarly passes CI
>
>  3. Marge tries to merge MRs A and B in a batch but finds that the
> combined 1% improvement in T1234 is *outside* the acceptance window.
> Consequently, the batch fails.
>
> This is quite painful, especially given that it creates work for those
> trying to improve GHC (as the saying goes: no good deed goes
> unpunished). 
>
> To mitigate this I would suggest that we allow performance test failures
> in marge-bot pipelines. A slightly weaker variant of this idea would
> instead only allow performance *improvements*. I suspect the latter
> would get most of the benefit, while eliminating the possibility that a
> large regression goes unnoticed.
>
To get things un-stuck I have disabled the affected tests on Darwin for
the time being. I hope we will be able to reenable these tests when we
have migrated fully to the new runners although only time will tell.

I will try to rebase the open MRs that are currently failing only due to
spurious performance failures but please do feel free to hit rebase
yourself if I miss any.

Cheers,

- Ben



signature.asc
Description: PGP signature
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: Changes to performance testing?

[John Ericson]

If we make the changes proposed in the "On CI" thread, I think we solve 
most of this for free. If a perf test fails, resubmitting should not 
need to rebuild GHC because it is cached. I'd say at that point, it's 
not even worth making Marge bot auto-accept extra improvements because 
restarting with new perf windows should be so easy.


John

On 2/22/21 8:46 AM, Andreas Klebinger wrote:


This seems quite reasonable to me.
Not sure about the cost of implementing it (and the feasability of it 
if/when merge-trains arrive).


Andreas

Am 21/02/2021 um 21:31 schrieb Richard Eisenberg:



On Feb 21, 2021, at 11:24 AM, Ben Gamari > wrote:


To mitigate this I would suggest that we allow performance test failures
in marge-bot pipelines. A slightly weaker variant of this idea would
instead only allow performance *improvements*. I suspect the latter
would get most of the benefit, while eliminating the possibility that a
large regression goes unnoticed.


The value in making performance improvements a test failure is so 
that patch authors can be informed of what they have done, to make 
sure it matches expectations. This need can reasonably be satisfied 
without stopping merging. That is, if Marge can accept performance 
improvements, while (say) posting to each MR involved that it may 
have contributed to a performance improvement, then I think we've 
done our job here.


On the other hand, a performance degradation is a bug, just like, 
say, an error message regression. Even if it's a combination of 
commits that cause the problem (an actual possibility even for error 
message regressions), it's still a bug that we need to either fix or 
accept (balanced out by other improvements). The pain of debugging 
this scenario might be mitigated if there were a collation of the 
performance wibbles for each individual commit. This information is, 
in general, available: each commit passed CI on its own, and so it 
should be possible to create a little report with its rows being perf 
tests and its columns being commits or MR #s; each cell in the table 
would be a percentage regression. If we're lucky, the regression 
Marge sees will be the sum(*) of the entries in one of the rows -- 
this means that we have a simple agglomeration of performance 
degradation. If we're less lucky, the whole will not equal the sum of 
the parts, and some of the patches interfere. In either case, the 
table would suggest a likely place to look next.


(*) I suppose if we're recording percentages, it wouldn't necessarily 
be the actual sum, because percentages are a bit funny. But you get 
my meaning.


Pulling this all together:
* I'm against the initial proposal of allowing all performance 
failures by Marge. This will allow bugs to accumulate (in my opinion).
* I'm in favor of allowing performance improvements to be accepted by 
Marge.
* To mitigate against the information loss of Marge accepting 
performance improvements, it would be great if Marge could alert MR 
authors that a cumulative performance improvement took place.
* To mitigate against the annoyance of finding a performance 
regression in a merge commit that does not appear in any component 
commit, it would be great if there were a tool to collect performance 
numbers from a set of commits and present them in a table for further 
analysis.


These "mitigations" might take work. If labor is impossible to 
produce to complete this work, I'm in favor of simply allowing the 
performance improvements, maybe also filing a ticket about these 
potential improvements to the process.


Richard

___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs


___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

MR template text

[Simon Peyton Jones via ghc-devs]

I often see MRs in my inbox that say

Thank you for your contribution to GHC!

Please take a few moments to verify that your commits fulfill the following:

[ ] are either individually buildable or squashed

This is because the author hasn't changed the Description of the MR, but rather 
has left the template text unchanged.
As a way to "nudge" authors to give reviewers more information, I suggest 
replacing the template text with the draft below.  Does anyone have any views, 
for or against?
Simon

Proposed new template

PLEASE REPLACE ALL OF THIS TEXT with a description of your merge request, 
including



* A description of what the Merge Request does.  For a single-commit MR, a copy 
of the

  commit message is often perfect.



* A reference (e.g. #19415) to the ticket that led to this MR, and that 
describes the

  problem that this MR solves.  Almost all MRs need a ticket, except the tiniest

  changes (e.g. code formatting)

  - A ticket describes a *problem*

  - A merge request describes a *solution* to that problem.



While you are encouraged to write a good MR Description, it's not compulsory.

You could just be putting up the MR to share with a colleague, for example.



But if you want (a) to get code reviews from others, or

(b) to land the patch in GHC,

please do follow these guidelines.



For general style guidance see

https://gitlab.haskell.org/ghc/ghc/wikis/commentary/coding-style


For completeness, the current template is


Thank you for your contribution to GHC!



Please take a few moments to verify that your commits fulfill the following:



* [ ] are either individually buildable or squashed

* [ ] have commit messages which describe *what they do*

   (referring to [Notes][notes] and tickets using `#` syntax when

   appropriate)

* [ ] have added source comments describing your change. For larger changes you

   likely should add a [Note][notes] and cross-reference it from the relevant

   places.

* [ ] add a [testcase to the

   
testsuite](https://gitlab.haskell.org/ghc/ghc/wikis/building/running-tests/adding).

* [ ] replace this message with a description motivating your change



If you have any questions don't hesitate to open your merge request and inquire

in a comment. If your patch isn't quite done yet please do add prefix your MR

title with `WIP:`.



[notes]: 
https://gitlab.haskell.org/ghc/ghc/wikis/commentary/coding-style#comments-in-the-source-code
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs