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

Add 'submission_id' and 'Kill Sub' columns in New submissions leaderboard #589

Merged
merged 5 commits into from
Sep 20, 2023

Conversation

frcaud
Copy link
Collaborator

@frcaud frcaud commented Sep 7, 2023

Adding a Kill Submission button in New submissions leaderboard to be able to terminate a submission.
First PR is adding a 'submission_id' column and adding a 'Kill Sub' column with clickable buttons on the HTML rendering of the New submissions leaderboard (leaderboard and My submissions pages on frontend)

Copy link
Collaborator

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @frcaud! That's the idea, though we need to figure out why CI fails and fix it.

@glemaitre
Copy link
Collaborator

My first guess (looking at the last history commit in main) would be to check the pytest. I would assume that this PR is using the latest pytest and they might have issue deprecation warning about the way we used the fixture and we did not change it on time.

@glemaitre
Copy link
Collaborator

glemaitre commented Sep 11, 2023

I see that pytest==6.2.5 does not raise the error for the fixture. So it should be something new.

The conftest.py file is here:

def database_connection():

@glemaitre
Copy link
Collaborator

It might be due to pytest-dev/pytest#11261

@glemaitre
Copy link
Collaborator

You need to add an empty pytest.ini in the root folder

@frcaud
Copy link
Collaborator Author

frcaud commented Sep 11, 2023

Thanks @glemaitre !
Frontend error is resolved.
Still some issue on test database

Copy link
Collaborator

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @frcaud ! For the failing test we need to edit test_get_leaderboard to add the extra column we added in,

>       assert (
            """<th>team</th>
          <th>submission</th>
          <th>submitted at (UTC)</th>
          <th>error</th>"""
            in leaderboard_failed
        )

assertion. The pytest log is a bit confusing, but I think that's actually the only failing test. The other exceptions you see in the logs, are the expected exceptions that are obtained when running a bad submission and can be ignored.

A few more comments below, otherwise LGTM.

@@ -427,12 +439,19 @@ def get_leaderboard(
columns = [
"team",
"submission",
"submission_id",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe let's just call it id so it would be a less wide column in the resulting html table? It's shown under a section of New submission, so there is no need to repeat "submission" in column names I think.

return f"<button onclick=\"alert(('You killed ' \
+ 'submission {cell_value}!'))\">{cell_value}</button>"

df["Kill Sub"] = df["submission_id"].apply(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again just "Kill" or maybe better "Stop" would be enough. It's in a table of submissions already.

competition_type = "public" if "public" in leaderboard_type else "private"
competition_type = (
"public" if "public" in leaderboard_type else "private"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if leaderboard_type not in ['new', 'failed'] we need to delete the extra column "id" we created before _compute_competition_leaderboard is run otherwise it will show in all other leaderboards

Copy link
Collaborator Author

@frcaud frcaud Sep 19, 2023

Choose a reason for hiding this comment

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

Since we create this extra column in elif leaderboard_type in ["new", "failed"]: it seems that we don't need to delete it in other leaderboard types, or am I mistaken ?

@frcaud
Copy link
Collaborator Author

frcaud commented Sep 20, 2023

Thanks @rth ! Tests OK. Seems there is one more error with codecov.

@frcaud
Copy link
Collaborator Author

frcaud commented Sep 20, 2023

CIs are green on rerun. Thanks @glemaitre and @rth !

@rth
Copy link
Collaborator

rth commented Sep 20, 2023

Thanks! Linting CI is still failing though. Can you run black on it?

@frcaud frcaud merged commit d6fc82d into master Sep 20, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants