scheduler for IOI problems #17
Conversation
There was a problem hiding this comment.
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
master/queue/jobgenerators/ioi_generator.go:230
- The NewIOIGenerator function returns an IOIGenerator without initializing its fields (such as givenJobs, groupNameToGroup, and groupNamesToBeGiven), which could lead to nil map references later in execution. Consider initializing these maps and any other necessary fields before returning the generator.
return &IOIGenerator{}, nil
master/queue/jobgenerators/icpc_generator.go:133
- [nitpick] The error message 'job %s not exist' is grammatically unusual; consider using a more conventional phrasing such as 'job %s does not exist'.
return nil, fmt.Errorf("job %s not exist", result.JobID)
grphil
left a comment
grphil
left a comment
There was a problem hiding this comment.
Ну пока половина ещё не дописана, но добавь парсинг групп в начале, и добавь баллы по группам чтобы было легко по группе смотреть, надо ли ее тестировать вообще. Мне кажется что подход когда ты в момент выдачи теста смотришь на группы, от которых тест зависит сильно проще смотрения вперед и генерирования джобов на будущее
| case verdict.OK: | ||
| if group.ScoringType == models.TestGroupScoringTypeEachTest { | ||
| if group.TestScore == nil { | ||
| logger.Panic("Group '%v' has type EachTest, but TestScore is nil in problemId=%v", |
There was a problem hiding this comment.
Вот тут не оч хорошо паниковать. Паниковать надо когда в нашем коде беды, а тут беды в настройке задачи. Вообще лучше заранее распарсь всю задачу и посмотри, все ли ок с настройкой. Если нет, то кидай CF
There was a problem hiding this comment.
О, по поводу CF, забыл важную вещь дописать. Я иногда на инвокере кидаю CF если у меня беды с инвокером, и в этом случае поле Error в InvokerJobResult заполнено чем-то. Я специально не загружаю эту штуку в сторадж, потому что это по факту отдельное сообщение об ошибке которое должно показываться только админу. Поэтому если тебе по задаче прилетел вердикт CF, то прочекай, есть ли что-то в поле Error, и если есть, то запиши в новое поле Submission эту ошибку. В это же поле можно писать ошибку если задача настроена плохо
There was a problem hiding this comment.
Вообще я хочу добавить полную проверку того, что задача нормальная, в начале newIOIGenerator
There was a problem hiding this comment.
В итоге просто перекладываю Error в посылку, мб добавляя что-то от себя (резолвни если норм)
misha567889
force-pushed
the
dev/scheduler/ioi
branch
from
76f0c60 to
f1bf2b0
Compare
misha567889
changed the title
misha567889
force-pushed
the
dev/scheduler/ioi
branch
from
ef50752 to
40dc6f8
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements updates for the scheduler to support IOI problems along with several naming and API consistency improvements for ICPC problem handling. Key changes include:
- Updating ProblemType naming conventions (removing underscores) for both ICPC and IOI problems.
- Introducing IOI generator support in the generator switch, along with refactoring common generator state.
- Enhancing models with new test group fields and updating JSON tags in submission and problem models.
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| master/queue/queue_test.go | Updated ProblemType usage in tests for consistent naming |
| master/queue/jobgenerators/icpc_generator_test.go | Removed the dedicated ICPC generator tests, affecting test coverage |
| master/queue/jobgenerators/icpc_generator.go | Refactored state typing and improved error message details |
| master/queue/jobgenerators/generator.go | Added IOI generator support through the switch statement |
| master/queue/jobgenerators/common.go | Introduced common generator state definitions |
| common/db/models/submission.go | Updated submission model with new JSON tag names and added GroupResults |
| common/db/models/problem.go | Revised ProblemType naming and added support for test groups |
| common/db/models/models_test.go | Extended auto migration tests to include the Problem model |
Files not reviewed (1)
- go.mod: Language not supported
Comments suppressed due to low confidence (2)
master/queue/jobgenerators/icpc_generator_test.go:1
- The complete removal of icpc_generator_test.go might reduce test coverage for ICPC-related job generation. Please ensure that equivalent tests are provided elsewhere if intended.
Entire file removed
common/db/models/submission.go:82
- Renaming the JSON field from 'Score' to 'GroupScore' may introduce breaking changes for API consumers; confirm that downstream clients are updated accordingly.
Score float64 `json:"GroupScore" yaml:"GroupScore"`
grphil
left a comment
grphil
left a comment
There was a problem hiding this comment.
Глобально мне очень не нравится разделение 0 и 1 нумерации, я бы везде юзал бы 1-нумерацию, как это во всех конфигах прописано, так будет сильно проще жить
grphil
left a comment
grphil
left a comment
There was a problem hiding this comment.
В целом сейчас уже мало что поменять надо, мне кажется на следующей итерации все сойдется и получится замерджить PR
| if job.Type != invokerconn.CompileJob { | ||
| return nil, fmt.Errorf("job type %s is not compile job", job.ID) | ||
| } | ||
| switch result.Verdict { |
There was a problem hiding this comment.
СF бывает если чето на так на инвокере, там ещё ошибка пишется в result.Error. Но пофиг, давай забьем пока, я потом когда буду админку мерджить (скорее всего после мерджа твоего ревью), добавлю это в админку
grphil
left a comment
grphil
left a comment
There was a problem hiding this comment.
Вообще конечно я мог бы подокапываться ещё, но пофиг, это важное ревью от которого много что зависит
No description provided.