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

Update canonical-schema.json with "timeout" property #1882

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

martinfreedman
Copy link

@martinfreedman martinfreedman commented Nov 8, 2021

Added non-required timeout (in milliseconds) property to labeledTest.properties to support tests with timeouts, such as for alphametics and palindrome-products exercises. This would support automatic generation/update of the tests rather than specific track manual modification of the relevant exercise's test file.

Added non-required `timeout` (in milliseconds) property to `labeledTest.properties` to support tests with timeouts, such as for `alphametics` and `palindrome-products` exercises. This would support automatic generation/update of the tests rather than specific track manually modification of the relevant exercise's test file.
Copy link
Contributor

@wolf99 wolf99 left a comment

Choose a reason for hiding this comment

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

I have no opinion on if this should be added. My review comment is purely on how to correctly add this if it is to be added

canonical-schema.json Show resolved Hide resolved
Added description reference for timeout property
Copy link
Contributor

@SaschaMann SaschaMann left a comment

Choose a reason for hiding this comment

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

How will that value be determined? The time it takes for a test to run will vary massively between languages, hardware, implementation.

It should also be added to the example in the README and documented there

@SleeplessByte
Copy link
Member

Do we expect a timeout? As in, do we want students to write code that times out?

@martinfreedman
Copy link
Author

@SleeplessByte For the test tooling that supports it, it is quite clear that this is a condition for a test to not time out and that a timeout is a test failure. The whole point is to make clear in the problem specification as to what is expected in the exercise as for the two already mentioned exercises. This is where it should be. Without this, the problem specification is inaccurate and/or misleading. Whether a specific track maintainer uses this property or not, they know it is part of the test constraints for a solution to pass.

Added a section to give an example of using the "timeout" property
@martinfreedman
Copy link
Author

@SaschaMann example added to Readme

@ErikSchierboom
Copy link
Member

I wonder if instead of this being property, it should instead be a scenario (see https://github.com/exercism/problem-specifications/blob/main/SCENARIOS.txt). That avoids any contention with how timeouts will be different between tracks, but you'd still be able to somehow mark a test case as being a performance-related test case (this is of course still contentious).

For the timeouts, I think the tests.toml file is a perfect place to define these timeouts.

@martinfreedman
Copy link
Author

martinfreedman commented Nov 12, 2021

I am happy to change to @ErikSchierboom 's suggestion using scenariosand tests.toml. IMV the key point is that these are performance constrained tests and this needs to be captured as this level of the exercise specification.

Shall I create a new PR and then link back this and then close this one?

@SleeplessByte
Copy link
Member

SleeplessByte commented Nov 12, 2021

@martinfreedman normally we would try to get some consensus, but you, Erik, and I now agree, so that's a consensus of at least three. Perhaps you want to discuss first what the scenario would be?

I would like to see: scenario: timeout or ineffecient or high-time-complexity and then we use error as usual. All of those would allow you to automate this in the test generator.

@ErikSchierboom
Copy link
Member

I am happy to change to @ErikSchierboom 's suggestion using scenariosand tests.toml. IMV the key point is that these are performance constrained tests and this needs to be captured as this level of the exercise specification.

Great!

I would like to see: scenario: timeout or ineffecient or high-time-complexity and then we use error as usual. All of those would allow you to automate this in the test generator.

Yeah, something like that would be nice. Another option for a scenario is performance. 🤷

@martinfreedman
Copy link
Author

I think "performance" scenario best captures what is being tested, whereas "timeout" states how to do that and can be a function of different languages and test frameworks. "Inefficient" and "high-time-complexity" also don't quite get to the fundamental point, that this is a performance test.

@SleeplessByte
Copy link
Member

I would be happy to accept "performance".

@ErikSchierboom
Copy link
Member

Works for me too!

@SaschaMann
Copy link
Contributor

SaschaMann commented Nov 16, 2021

I don't like performance for this. performance suggests to me that it could also be about optimising the implementation in the language to make it faster without changing the algorithm, whereas the exercises you mention are primarily about algorithmic optimisations. The former would require significantly more track-specific changes, so I think the distinction is important.

(the change request is about the previous comments, not the name of the scenario)

@ErikSchierboom
Copy link
Member

(the change request is about the previous comments, not the name of the scenario)

Eh, are you against or in favor of using performance as the scenario?

@SaschaMann
Copy link
Contributor

Eh, are you against or in favor of using performance as the scenario?

Against it and wanted to explain why because the reason wasn't brought up yet, but if people disagree with that reason, it's fine by me, too.

Just wanted to clarify what the blocking change request is about.

@ErikSchierboom
Copy link
Member

Do you have an alternative name perhaps?

@SaschaMann
Copy link
Contributor

Maybe something like algorithmic-optimisation or algorithmic-optimisation-needed?

@martinfreedman
Copy link
Author

I don't like performance for this. performance suggests to me that it could also be about optimising the implementation in the language to make it faster without changing the algorithm, whereas the exercises you mention are primarily about algorithmic optimisations. The former would require significantly more track-specific changes, so I think the distinction is important.

I disagree. Performance covers both and that is why I like it. Plus different languages might have different facilities. The exercises are both about combinatorics and that is likely to be the main use case for any exercise for these performance tests. Having built-in fast or optimised combinatorics alone will not or should not help.

@SaschaMann
Copy link
Contributor

Performance covers both and that is why I like it.

That's precisely why I think it's unsuitable. Both cases require different handling in the implementation, which is why the broad term is too general for a scenario.

@martinfreedman
Copy link
Author

I do not follow this since, both scenarios (if there are really two , not sure) require same handling in a test, in the case of F# & C# by using the xunit timeout attribute but still keeping the same test name.

@ErikSchierboom
Copy link
Member

@exercism/reviewers Your thoughts on what the scenario should be named?

@SaschaMann
Copy link
Contributor

I do not follow this since, both scenarios (if there are really two , not sure) require same handling in a test, in the case of F# & C# by using the xunit timeout attribute but still keeping the same test name.

They might require the same handling in a test but not necessarily in the changes to the instructions. Using different scenarios adds that additional information so that human maintainers can make use of it, even if auto generators handle them identically.

@BethanyG
Copy link
Member

BethanyG commented Dec 9, 2021

I am somewhat ambivalent to the name, as long as it gives me a clean way to flag tests that could be problematic, rather than turning off the runner altogether for an exercise. I favor any method that will also make it possible for me to add in explanation/warning for students. Something along the lines of:

"While you can use a naive or 'brute force' methods to solve this exercise, large inputs may cause processor or memory issues. The following tests are designed with large inputs and may timeout, fail, or cause problems if your code is inefficient."

or

"This exercise can be solved using recursion, but carefully check your code. Python does not have tail-call optimization, and larger inputs could cause problems. The following tests are designed with large inputs and may timeout, fail, or cause other issues if your solution is inefficient."

FWIW, Pytest (what we're using more or less for the Python test runner) has a slow flag, intended to be used for "resource-heavy" tests. That's probably what I'd map any of these terms to once they're in place.

To me, performance implies hitting a certain speed or processor or memory benchmark. Then again, with our test runner timeouts, we are indeed implying that there is one.

@ErikSchierboom
Copy link
Member

I quite like "slow"!

@ErikSchierboom
Copy link
Member

What do other people think of "slow" as the scenario name?

@martinfreedman
Copy link
Author

Given the point that @SaschaMann made namely " whereas the exercises you mention are primarily about algorithmic optimisations." how about Optimise?

@SaschaMann
Copy link
Contributor

At the risk of continuing to bikeshed: I don't like slow for the use cases outlined in this PR. Not all test cases that might require timeout handling of some sort are inherently slow. There are cases where a naïve implementation might be slow yet a solution that implements a more efficient algorithm won't be. In that case, tagging the case as slow doesn't really make sense to me but that's what we'd be doing in the canonical data.

@martinfreedman you've also downvoted the suggestion, could you elaborate why?

@ErikSchierboom
Copy link
Member

Seeing that @SaschaMann and @martinfreedman have downvoted "slow", what do people things about the suggested "optimise"?

@kotp
Copy link
Member

kotp commented Jan 11, 2022

I like optimized, but it leaves the question unanswered for "optimized for what? Time, Memory, Energy, Readability?"

@martinfreedman
Copy link
Author

martinfreedman commented Jan 14, 2022

I like what @BethanyG implies "Efficient" better than "Optimise" (and especially not "Optimize"). Or maybe "TimeEfficiency" as that is the specific issue I am addressing and offering a solution for.

In addition the expected BigO restriction could be added into a test name e.g. "Large Inputs require at least Logarithmic Performance" ) or (Linear or LogLinear etc.)

@ErikSchierboom
Copy link
Member

Maybe time-optimization or optimize-time?

@martinfreedman
Copy link
Author

@ErikSchierboom you mean "time-optimise" or "optimise-time". Most of us are not from the USA, why should we have the imposition of USA spelling?

@ErikSchierboom
Copy link
Member

Most of us are not from the USA, why should we have the imposition of USA spelling?

Because that's what our style guide defines :)

@martinfreedman
Copy link
Author

@ErikSchierboom OK, Then I like "optimize-time"

@ErikSchierboom
Copy link
Member

Cool! @SaschaMann @kotp @BethanyG @SleeplessByte @wolf99: what do you think about optimize-time?

@SaschaMann
Copy link
Contributor

I would prefer if it specified that this is about time complexity, not just time to run in general, but I'd be happy with either.

@ErikSchierboom
Copy link
Member

I'd be perfectly fine with time-complexity or some variant.

@kotp
Copy link
Member

kotp commented Jan 15, 2022 via email

@BethanyG
Copy link
Member

I'm fine with time-complexity, optimize-time/time-optimize, or time-efficiency. 😄

@SaschaMann
Copy link
Contributor

I'd be perfectly fine with time-complexity or some variant.

I was thinking optimise-time-complexity, I like optimise-time but on its own it has the same issues that I described above where it's not clear what one needs to optimise.

@ErikSchierboom
Copy link
Member

Okay, anyone against using optimise-time-complexity then? :)

@ErikSchierboom
Copy link
Member

Okay, one final chance. Does anyone object to using optimise-time-complexity for the name of the scenario?

@ErikSchierboom
Copy link
Member

Okay, let's use optimise-time-complexity then. @martinfreedman could you update the PR (it also needs rebasing)?

@IsaacG
Copy link
Member

IsaacG commented Jan 1, 2023

@martinfreedman Are you still interested in working on this?

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.

8 participants