Concerns about the current status quo around internal PRs

[zakkak]

Hello all,

I would like to start a discussion about the current status quo regarding internal PRs and some concerns that the Mandrel team has about it.

It appears that a number of internal PRs end up on the upstream repository through direct pushes to master, some examples are:

• Deprecation of --initialize-at-build-time= 5013ca7
• Filtering out unused security providers 10cc640
• Single parsing of bytecode e92dab0

Our concern is that the sudden appearance of such changes on master makes it hard:

• for community contributors to keep track of what's going on and link/reference issues to the corresponding changes,
• for framework maintainers to ensure that their framework will be ready for the next GraalVM release, and
• for the community to participate in the discussion about some decisions that might affect them.

Furthermore, it allows internal PRs to bypass the github CI gates which might lead to breaking the github CI gates like in #3433 and #3423.

It looks like there is an option for internal PRs to be "mirrored" on github (e.g., #3271), which could help with the above concerns, but it's not enforced at the moment.

[thomaswue]

Agree on the first two items that they need to be discussed in a PR. Also agreed that the GitHub gates should not be allowed to fail.

[christianwimmer]

It is interesting that the 3 examples you mention are all from a separate category of improvements:

• Deprecation of --initialize-at-build-time= is sort-of a necessary cleanup based on "false positive" GitHub issues that increased over the last months
• Automatic discovery of security provides was on the published compatibility roadmap Remove some limitations of GraalVM Native Image #2762
• Single parsing of bytecode is an internal architecture improvement without user-visible functional changes.

While we can always write up more "roadmap" issues, the timing when something lands is not really predictable. Sometimes developers just get motivated and fix something that we know was a long-standing architecture improvement.

But I agree that we can communicate more and better. What about a regular "Native Image Developer Meeting" where we present what happened and what is going to happen? I like that better than writing up long pieces of text that get stale quickly, and it allows us to provide a outlook and status without formalizing too much. This would be a code-driven technical meeting.

[thomaswue]

It is a trade-off between slowing down the work of the core team - which is responsible for maintenance and is also the first receiver of any bugs reported on GitHub - and providing more info to contributors who are often only interested in a certain aspect of the project and will also not take over longterm maintenance of whatever is merged.

[maxandersen]

It is interesting that the 3 examples you mention are all from a separate category of improvements:

I want to highlight these are not the only ones - we've seen many more in the past; these are just recent one that we managed to notice. This is part of challenge - us (mandrel/quarkus teams) feeling the need to keep track of every commit rather than just keep an eye on PRs / issues coming through. A big thing of most open source projects is the trust that you know a commit can be traced back to issue / PR with more context letting you grok why something changed.

While we can always write up more "roadmap" issues, the timing when something lands is not really predictable. Sometimes developers just get motivated and fix something that we know was a long-standing architecture improvement.

Yes - and we do the same ourselves in our projects; but we do always have a related issue or PR connected to the changes. I'm confident you have references inside the internal issue tracking system of Oracle but that must be considered secondary not the primary way to document/track changes.

Also note; we are not asking to write more "roadmap" issues in this; we are asking to at least let changes go through the PR system so there is a natural place for feedback before and after the work been merged.

What about a regular "Native Image Developer Meeting" where we present what happened and what is going to happen?

+1 to this but it does not fix the issue of direct to master commits.

It is a trade-off between slowing down the work of the core team - which is responsible for maintenance and is also the first receiver of any bugs reported on GitHub

Are you saying that the core team should not be expected to use issue and PR's when committing to master even oracle internal system ? Please note we are not here asking to require PR to open for N days before merging - we are asking to at least make things traceable.

providing more info to contributors who are often only interested in a certain aspect of the project and will also not take over longterm maintenance of whatever is merged.

Afaics we are here to help maintain longterm and the traceability and open communication we are asking about here should be a benefit for you too.

[thomaswue]

If a commit fixes a GitHub issue or related to a public road map issue, the policy is to mention said issue in the commit message and also post a comment on the issue with a link to the commit. This should make changes related to public GitHub issues traceable.

We can write a script that automatically creates a PR for every change merged into master and a "LGTM!" comment from some reviewer. I don't think this would resolve your concerns though. I think what you would like to see is that the team creates themselves detailed GitHub issues before committing a change.

There are many changes merged everyday in all areas of the project. Your team will only be interested in a fraction. I think a native image developer meeting will serve best in terms of focusing on the core issues instead of adding lots of process for every change.

[zakkak]

If a commit fixes a GitHub issue or related to a public road map issue, the policy is to mention said issue in the commit message and also post a comment on the issue with a link to the commit.

Indeed, I have seen the latter happening with only a few exceptions, but the former is not that common nor enforced to community contributors.

Another, minor I would say, issue related to this is that there is no way to see from a commit that doesn't reference an issue in it's message what PR/issue it links to. For instance #2680 got rebased and merged, as the rebase was not pushed back on github @olpaw had to add a link to the commit manually. This works fine if you find the PR and want to see the corresponding commit, but it does not work the other way around (i.e., there is no way to see #2680 from e17b00a, while you can do this for merged GH PRs, e.g. for #3468 and 6deecfd.

We can write a script that automatically creates a PR for every change merged into master and a "LGTM!" comment from some reviewer. I don't think this would resolve your concerns though. I think what you would like to see is that the team creates themselves detailed GitHub issues before committing a change.

Indeed, that would be ideal. But I would also be happy with PRs with a descriptive title and a short description only where needed, e.g., for 5013ca7 the commit message could be used as a title and a short justification could follow that would explain the reasoning behind this decision, while for 10cc640 just the title would suffice as the documentation update makes clear what this change does (although it would be nice to also include some references to the issues it tries to solve/mitigate).

Even if we didn't have the time to react before the merge of a PR we would still be able to start a discussion in the PR keeping things in a single place for future reference. This is even more important for larger changes where one might want to comment on the code changes (something we can do in individual commits, but not with the same UI support).

There are many changes merged everyday in all areas of the project. Your team will only be interested in a fraction.

That's true, but are we the only community contributors? What about truffle community contributors, sulong, etc.?

I think a native image developer meeting will serve best in terms of focusing on the core issues instead of adding lots of process for every change.

The native image developer meeting sounds good indeed, but I would see it as complementary.

My guess is that you already have such processes happening internally, so one way to reduce the overhead would be to try and move as much of the processes as possible to the open (the GitHub repo in this case).

[borkdude]

There is an open discussion about --initialize-at-build-time here:

#3476

I'm looking forward to some information on this, as this is quite concerning for Clojure projects.

[maxandersen]

Also commits to master without running the public tests just seems like a really bad practice.

[maxandersen]

I'm sorry but that really don't hold any value outside oracle. For example we can't in mandrel perform the same tests before our release.

An opensource project is no better than what anyone can run/test.

[christianwimmer]

I'm sorry but that really don't hold any value outside oracle.

This statement is wrong. Our internal gates just run mx gate, and all of that is available to you to run inside RedHat too.

[thomaswue]

Yes, of course we run the public tests before allowing code to reach master. We are making it a top priority to fix issues associated with the GitHub testing infrastructure that makes it appear as if the tests are failing. I see that Quarkus has the same problems with recent merges showing failing tests like at https://github.com/quarkusio/quarkus/runs/2869480051. How does this code get into the main line of Quarkus obviously bypassing the GitHub CI gates @zakkak @maxandersen?

[maxandersen]

Quarkus PRs pull requests and issues are documented and being discussed in public. It's traceable what have it have not happened. If you see we missed something have issues with specifics in quarkus or any other opensource project please open a concern on the appropriate list/issue tracker.

This is the main difference here. Oracles direct commits and internal test runs stands out as separate from other contributors workflow.

There will occasionally be cases / periods where things are not smooth - that is a natural part of software development.

As Dinn says - We are looking to make collaboration and work easier here. A Part of that is To document (in this case mainly automatically) and make traçable the development cycle as it happens not only the "curated highlights".

.

[zakkak]

I see that Quarkus has the same problems with recent merges showing failing tests like at https://github.com/quarkusio/quarkus/runs/2869480051.

I am not going to argue that Quarkus main never breaks, that would be awesome. AFAIK the Quarkus team is doing its best to avoid this though. To the best of my knowledge the only times when the Quarkus team did allow commits to go in without passing tests was during a period of time when the Quarkus' github workflow was highly unstable and the team needed to make progress despite of that, but this is no longer the case.

How does this code get into the main line of Quarkus obviously bypassing the GitHub CI gates @zakkak @maxandersen?

The specific failure seems to be due to some github infrastructure issue and not due to code issues.
When the corresponding PR was tested (before merge) all checks were green, so that is how it got into the main branch, n.b. without bypassing the GitHub CI gates or being pushed directly to the main branch.

As we see in the screenshot bellow, with the next merge all tests were back to green again, although the merged PR was not fixing something related to the last failure (hinting that it was probably a false negative). It's also worth noting the huge difference in execution time, it's almost double in the failing case.

FWIW in quarkusio/quarkus#16631 I present some issues with Quarkus' CI and outline a plan on how to improve it (part of which has been implemented). Any feedback is welcome.

[maxandersen]

so do i get it right - Oracle employees working on graalvm are okey to commit directly to master without public nor internal trace of the work ? I just want to make sure I understand the rationale for why not just opening a PR for these changes?

just to compare graal has ~54K commits since 2011 (10 years), Quarkus has ~22K commits since 2018 (3 years) - every commit except possibly the very early days have a PR/issue with at least one reviewer connected. That goes for every core comitter and contributor. generally no excuses unless exceptional scenario.

Thus if you are arguing doing PRs for commits hold you back I'll argue you must be doing something we don't do - as I would not call Quarkus as being held back. I would really love to understand and help here where we can.

[thomaswue]

The trace is the series of commits landing in master. The general policy is certainly that the sources committed to master and associated commit messages must be self-explanatory; i.e., no side structure is required to interpret the changes.

Quarkus seems a GitHub project with an exceptionally large amount of PRs (10k+); but many of them seem auto-generated or with minimal or zero commentary. In majority of cases, the commit message contains all the relevant information; and this is also what I would expect from a project. When cloning the project, I want to be able to understand sources and commit history based on commit messages without having to look up a PR.

We can also auto-generate PRs if # of PRs is what we should be optimizing for. If most of your commits look like this it is easy to have a high number ;-) quarkusio/quarkus#17904, quarkusio/quarkus#17943

It seems also here that it is possible to commit to master in Quarkus with only a single engineer involved, because the bot creates the PR, an engineer presses "OK" and then it is merged?

[thomaswue]

The commit of the referenced dependabot PR has the same information as the PR: quarkusio/quarkus@e0b2f89

This is why I don't quite understand "PR is 10x better than commits". For me, a PR is a set of commits; and the messages (and source code comments) of those commits must be sufficient to understand what was changed & why. I believe it is the general spirit of open source, that said source is self-explanatory and does not require other channels to receive information from. I saw for example PRs in Quarkus that mention some conversation on a non-public chat app, so one would have to create an account there and sign in, and I do not think that this should be required.

There is certainly no slow-down if all that is happening is that a bot creates a GitHub PR and copy&pastes the commit messages of what is merged there. The only point where any of this makes a difference is when a PR is blocked - i.e., when a PR acts as a manual gate before sources are committed to the main line. I understand that the developer experience with manual PRs by external contributors is actually painful, because of this - i.e., PRs getting delayed for a long time. I would prefer that external contributors have an easier way to merge into main line, and this kind of tooling is what we are currently working on.

[maxandersen]

Let me be frank - direct commits to master with zero conversation elsewhere is viewed by many as arguments for that oracle works around the community rather than within it.

Having PRs creates a plqce for each commit to take pause; do checks etc. Before going to master but even if goes directly through pr review the mere presence of a PR rather than having to track every commit is better.

Ps. Please tell me where you see discussion pointing to non public chats. We use zulip which is public. If we did refer to something non public without explanation/context that is a mistake. Exactly why we try make everything via public issues/PRs so less risk of such to happen.

[maxandersen]

Also my assumption is we are working together to eventually have non-oracle employees committing and recovering to graal codebase - Im assuming you would not be fan of commits directly without context either in that case?

[christianwimmer]

Let me be frank - direct commits to master with zero conversation elsewhere is viewed by many as arguments for that oracle works around the community rather than within it.

Please stop this "RedHat is so much more open than Oracle". That is all marketing.

If RedHat wants to be seen as a credible contributor to GraalVM Native Image, you need to e.g. start fixing bugs instead of reporting bugs. So far I have seen lots of complaints but not so much of contribution (JFR and debug info support are notable exceptions - so I have hope for the future).

Let me be frank too: you gain credibility by contributing, not by complaining.

[maxandersen]

I'm sorry you are seeing this as complaining - we have seen great improvements over the last year on how development happens around GraalVM it is what enabled us to contribute JFR and debug info. Thus thank you for that!

Before that ARM support and there is also a list of various performance improvements and the started exploration around garbage collection option. There is the adoption of Mandrel to have support on the stock OpenJDK plus adjustments in code outside Graal like in OpenJDK itself, Quarkus and lots of libraries where we work to enable the code to work the best possible way with native-image.

This list is not to say one is contributing more than another - it is to highlight that we are and plan to continue contributing fixes and new feature work - is just that code contribution is not everything that is needed for that - being able to grok what is happening and what actually did happen between builds we find incredibly valuable in being able to contribute in the first place. Raw git logs does not scale well for that.

Hence we simply based on our experience in contributing to Graal suggested to discuss how we could reduce the need of times where we see changes popup and not be able to easily grok it without contacting developers personally. This creates a lot of unnecessary tension and uncertainty.

If there was a way to submit a PR for that we would - having a discussion about it was our attempt to contribute to improve.

We suggested this to reduce complaints not increase it.

Peace!

[thomaswue]

@maxandersen Zulip is not public, it is behind a login (https://quarkusio.zulipchat.com/login/). My general point is just that no other system or source of information should be required to understand a change other than appropriate commit messages and code comments. It was for example a concern for us that when RedHat suggested a PR for JFR support, the commit message in that PR did not contain much information; and there was no commit history or breakdown of who of the five authors did what kind of changes in those 10k LOC (13292c2).

There was information in the PR, but in my point of view that information should have been primarily in the commit message - or better in the multiple commit messages of the individual commits that document how the code was created. It is still unclear to me what source code system (and individual PRs with discussions and explanations) were used by those five engineers when developing this. Is this somewhere publicly accessible?

The conditions for merging are the same for all contributors - i.e., commit messages and source code must be understandable without extra context. Changes are ideally split across smaller commits where each explains a step.

[zakkak]

@maxandersen Zulip is not public, it is behind a login (https://quarkusio.zulipchat.com/login/). My general point is just that no other system or source of information should be required to understand a change other than appropriate commit messages and code comments.

Thanks for pointing this out Thomas.
I have opened https://github.com/quarkusio/quarkus/issues/18068 to bring this to the attention of the Quarkus team.

[maxandersen]

Thanks @zakkak !

[adinn]

I think it would be a good idea to take some of the heat out of this discussion and get back to why Foivos originally raised the issue. He said

Our concern is that the sudden appearance of such changes on master makes it hard:

for community contributors to keep track of what's going on,
for framework maintainers to ensure that their framework will be ready for the next GraalVM release, and
for the community to participate in the discussion about some decisions

These are actually legitimate concerns that come out of our experience. They do not really have anything to say about who is doing open source better than anyone else. They are concentrated on the ability of non-Oracle contributors to Graal and community users of Graal (especially middleware devs) to coordinate their work with Oracle contributors.

A public PR, preferably opened early during development of an internal fix rather than at the point of commit, would help us to know what is being changed and plan for it. I don't think that's a great deal to ask of Oracle's developers but it will help us to make faster and better joint progress. Could we return the discussion to this important core point and see if it would be possible to try to improve this fairly simple element of team working?

[borkdude]

@thomaswue That would be pretty useful! I can only see 22 closed issues, is that approximately right? It seems on the low side. I don't see an issue related to the bug I had.

[maxandersen]

One thing that list misses are the direct commits like the ones that started this conversation.

Our suggestion to improve that is to always have a PR matching (with milestone set) since then it is easier traceable than what raw commits provided.

For PRs set of milestone can even be automated based on what branch it gets merged to.

[thomaswue]

@Addin @maxandersen So we have a detailed native image road map showing what is currently worked on and what is considered as todo at https://github.com/oracle/graal/projects/2. Items on this road map should give advance notice of what future changes are expected. We would actually also be happy to share some of the work related to the todo items. Then we also started doing PRs where we explicitly ask for feedback like at #3473. Also here another public PR by the team reacting quickly to a reported issue by @zakkak at #3440. Then Christian is suggesting as a response to @zakkak a regular developer meeting, which I think is a great idea.

My genuine believe is that:
a) Source code and commit messages should contain all the relevant information without the requirement to reference any other system (PR, chat app, or similar). This is btw a general policy for the project. I understand it is not always followed; but I am happy to contact and discuss with any author of a commit where this is not the case.
b) Instead of having some global policy with lots of requirements (like "you must open a PR for every commit") it will lead better results if the main changes are appropriately highlighted in the road map as well as in the developer meeting. Bureaucratic requirements can quickly lead to just many cases where the rules are followed to the letter, but no real improvement is made.

The discussion of changes early and the detailed commit messages are not only some policy for the team, but this also applies to external contributions. We had several cases in the past where some big set of changes pops out of nowhere with a single squashed commit mixing work from 5 authors (see the JFR PR as an example at 13292c2).

The core team has a lot of fixes to do. If we see more help with these issues, it will certainly make a good case to do more work to enable those contributions. I understand this could be a chicken & egg problem. But as such, I do think this can then only be solved with gradual improvements. And I believe we had quite a lot such improvements over last year.

[Addin]

OMG Wrong tag @thomaswue 🤣

[thomaswue]

Haha, sorry about that; meant of course @adinn ;-)