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

Transformer ranking part 1 #562

Merged
merged 73 commits into from
Apr 25, 2024

Conversation

nitbharambe
Copy link
Member

No description provided.

nitbharambe and others added 12 commits April 3, 2024 19:54
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@nitbharambe nitbharambe marked this pull request as ready for review April 8, 2024 06:57
@nitbharambe nitbharambe marked this pull request as draft April 8, 2024 06:57
Copy link
Contributor

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo left a comment

Choose a reason for hiding this comment

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

I like the spirit ;) (of skipping the tests)

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo added the feature New feature or request label Apr 8, 2024
@TonyXiang8787
Copy link
Member

@nitbharambe @Jerry-Jinfeng-Guo why is #567 merged but this not? How can you test the ranking without the full implementation?

@Jerry-Jinfeng-Guo
Copy link
Contributor

@nitbharambe @Jerry-Jinfeng-Guo why is #567 merged but this not? How can you test the ranking without the full implementation?

The merged tests are for consequtive steps in the ranking. Since steps 2-3-4 and step 1 have inter-dependency, so are the tests. Therefore we figure it to be a good idea to merge stuff from 2-3-4 once ready and continue working on 1, while cross-referencing.

@Jerry-Jinfeng-Guo
Copy link
Contributor

CI is stuck

@mgovers
Copy link
Member

mgovers commented Apr 11, 2024

CI is stuck

that's because @nitbharambe prefixed the last commit 847723d with [skip ci]

image

@Jerry-Jinfeng-Guo
Copy link
Contributor

CI is stuck

that's because @nitbharambe prefixed the last commit 847723d with [skip ci]

That should skip it, shouldn't it?

Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
@mgovers
Copy link
Member

mgovers commented Apr 11, 2024

CI is stuck

that's because @nitbharambe prefixed the last commit 847723d with [skip ci]

That should skip it, shouldn't it?

yes but the way github implemented it is that if there are Required checks (configurable in the repo admin settings) then it will always mark them as Expected.

This also is the reason why the Publish job is awaited in the merge queue but it doesn't link to the pipeline that will set it (because the job is the last one in the pipeline and won't report its status until it has started). I've been thinking about actually adding a report step that tells the merge queue that the pipeline that will set run those checks, but haven't come up with a sustainable solution yet

image

@TonyXiang8787
Copy link
Member

CI is stuck

that's because @nitbharambe prefixed the last commit 847723d with [skip ci]

That should skip it, shouldn't it?

yes but the way github implemented it is that if there are Required checks (configurable in the repo admin settings) then it will always mark them as Expected.

This also is the reason why the Publish job is awaited in the merge queue but it doesn't link to the pipeline that will set it (because the job is the last one in the pipeline and won't report its status until it has started). I've been thinking about actually adding a report step that tells the merge queue that the pipeline that will set run those checks, but haven't come up with a sustainable solution yet

image

We can just push a new commit with skip ci in the message right?

@mgovers
Copy link
Member

mgovers commented Apr 11, 2024

CI is stuck

that's because @nitbharambe prefixed the last commit 847723d with [skip ci]

That should skip it, shouldn't it?

yes but the way github implemented it is that if there are Required checks (configurable in the repo admin settings) then it will always mark them as Expected.
This also is the reason why the Publish job is awaited in the merge queue but it doesn't link to the pipeline that will set it (because the job is the last one in the pipeline and won't report its status until it has started). I've been thinking about actually adding a report step that tells the merge queue that the pipeline that will set run those checks, but haven't come up with a sustainable solution yet
image

We can just push a new commit with skip ci in the message right?

yes but that is not the issue 😉 my second comment was only related to the merge queue

nitbharambe and others added 7 commits April 23, 2024 01:19
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

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

part 1 of review

Jerry-Jinfeng-Guo and others added 3 commits April 24, 2024 11:34
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Signed-off-by: Nitish Bharambe <nitish.bharambe@alliander.com>
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

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

almost there 😬

Jerry-Jinfeng-Guo and others added 2 commits April 25, 2024 09:46
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

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

FINALLYYYYYYY 🎉

Copy link

sonarcloud bot commented Apr 25, 2024

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo added this pull request to the merge queue Apr 25, 2024
Merged via the queue into main with commit 09ee5a6 Apr 25, 2024
26 checks passed
@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo deleted the feature/transformer-tap-ranking-part-1 branch April 25, 2024 14:16
@mgovers mgovers mentioned this pull request Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants