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 ~~memory~~ limits #415

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

add ~~memory~~ limits #415

wants to merge 17 commits into from

Conversation

mzuenni
Copy link
Collaborator

@mzuenni mzuenni commented Jan 23, 2025

implement some more limits

Copy link
Collaborator

@mpsijm mpsijm left a comment

Choose a reason for hiding this comment

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

Haven't tested it yet, but here are some first comments based on the code 😄

bin/run.py Outdated Show resolved Hide resolved
bin/program.py Outdated Show resolved Hide resolved
@RagnarGrootKoerkamp
Copy link
Owner

RagnarGrootKoerkamp commented Jan 23, 2025

No time to look at the code yet, but I could imagine the -m argument still being useful to override the yaml key for quick testing purposes?

@mzuenni
Copy link
Collaborator Author

mzuenni commented Jan 23, 2025

but should the yaml key override it or be an additional limit (i.e. we take the min of both) and should that be for submissions or all subprocesses... ?

@mzuenni mzuenni changed the title add memory limit add ~~memory~~ limits Jan 24, 2025
@mzuenni mzuenni requested a review from mpsijm January 24, 2025 16:54
@mzuenni
Copy link
Collaborator Author

mzuenni commented Jan 29, 2025

I think everything should work now, @mpsijm do you have time to give it a try? :)

Copy link
Collaborator

@mpsijm mpsijm left a comment

Choose a reason for hiding this comment

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

I've used your branch today when preparing some problem ideas for the Freshmen Programming Contests, and it didn't crash 😄 I used at least bt run, bt generate, bt validate, bt constraints, and bt fuzz. Note that I haven't actually touched any of the new limits yet, nor did I explicitly try to push a submission/generator/validator past the time/memory/output limit (I only had some TLE submissions). But some testing is better than no testing, I guess 😛

bin/generate.py Outdated Show resolved Hide resolved
bin/problem.py Show resolved Hide resolved
bin/program.py Show resolved Hide resolved
Comment on lines +501 to +504
if 'timeout' not in kwargs and 'timeout' in self.limits:
kwargs['timeout'] = self.limits['timeout']
if 'memory' not in kwargs and 'memory' in self.limits:
kwargs['memory'] = self.limits['memory']
Copy link
Collaborator

@mpsijm mpsijm Jan 31, 2025

Choose a reason for hiding this comment

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

How can the Program class know whether the timeout and memory keys in self.limits are the ones that it should use? It could be that we need to use validation_memory or some other key for the timeout/memory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is why we use self.limits not problem.limits the constructor of the subclass interferes the right values to use and passes it to the super class

Copy link
Collaborator

@mpsijm mpsijm Feb 1, 2025

Choose a reason for hiding this comment

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

Ah right, I got confused between the two kinds of limits, thanks 😄 Maybe we can give Program.limits a different name than Problem.limits to avoid confusion, but I'm not sure what 😅

A different suggestion could be to not store the five types of limits for a Program in a dict, but to have explicit fields (self.timeout, self.memory, self.compilation_time, etc.)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but all of these are optional? (and maybe None needs to be handles differently at some point?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

They already are optional currently, right? The only difference is that you use self.limits.get(<key>, <default>) to fetch the values from the dict, falling back to <default> if no value exists for the <key> 😛

I think it would be nice if we set the defaults in the constructor already, instead of spread throughout the Program class. Then, the limits dict only needs to exist in the constructor, and the fields (self.timeout, self.memory, self.compilation_time, etc.) are no longer optional 😄 And that also makes it easy to change potential future differnt handling of None, because that all stays within the constructor 🙂

(even if we don't split up Program.limits to five separate fields, we still may want to set the defaults in the constructor)

doc/commands.md Outdated Show resolved Hide resolved
bin/verdicts.py Outdated Show resolved Hide resolved
bin/problem.py Show resolved Hide resolved
bin/problem.py Show resolved Hide resolved
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