Add parallel letter frequency #1638
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ccadden
force-pushed
the
add-parallel-leter-frequency
branch
2 times, most recently
from
ad12275 to
091c8b8
Compare
kotp
reviewed
Feb 23, 2024
exercises/practice/parallel-letter-frequency/parallel_letter_frequency_test.rb
Outdated
Show resolved
Hide resolved
kotp
reviewed
Feb 23, 2024
ccadden
force-pushed
the
add-parallel-leter-frequency
branch
2 times, most recently
from
0f6526c to
cabb8ab
Compare
ccadden
force-pushed
the
add-parallel-leter-frequency
branch
3 times, most recently
from
644adcc to
8a0c9ee
Compare
Do we or should we show this as part of the exercise until this is no longer considered experimental? |
kotp
reviewed
Feb 24, 2024
exercises/practice/parallel-letter-frequency/parallel_letter_frequency_test.rb
Outdated
Show resolved
Hide resolved
ccadden
force-pushed
the
add-parallel-leter-frequency
branch
from
e83d940 to
ec60c84
Compare
10 tasks
ccadden
force-pushed
the
add-parallel-leter-frequency
branch
3 times, most recently
from
5d85c7b to
31ccfc2
Compare
| class ParallelLetterFrequency | ||
| def self.count(texts) | ||
| ractors = (0...texts.length).map do |i| | ||
| Ractor.new(texts[i]) do |text| |
There was a problem hiding this comment.
Is Ractor a gem that is not installed by default?
There was a problem hiding this comment.
Ractor may be a gem, but it is included in the core/standard libraries.
There was a problem hiding this comment.
I wonder of the *.txt files might best be moved to their own directory (inputs or texts or something like that).
There was a problem hiding this comment.
Moved files to data directory.
There was a problem hiding this comment.
I think it might be confusing for students to see this file when they download the directory. I would suggest copying the code to a method in the test class and then to remove this file.
There was a problem hiding this comment.
I agree with this, it is a helper method for the benefit of the tests, not something that should be outside of the test.
There was a problem hiding this comment.
Moved class file to helper method within the tests.
There was a problem hiding this comment.
Could you remove this file?
There was a problem hiding this comment.
This should be treated like the other files that are used for processing. Just because it is about Ruby does not change the idea that it is just a data file of words for processing. (Stroop Effect in play here?)
Comment on lines
167
to
181
| def test_faster_than_serialized_answer | ||
| skip | ||
| texts = Array.new(10, 'a' * 100_000) | ||
|
|
||
| GC.start | ||
| t0_parallel = Minitest.clock_time | ||
| ParallelLetterFrequency.count(texts) | ||
| parallel_time = Minitest.clock_time - t0_parallel | ||
|
|
||
| t0_sequential = Minitest.clock_time | ||
| SequentialLetterFrequency.count(texts) | ||
| sequential_time = Minitest.clock_time - t0_sequential | ||
|
|
||
| assert parallel_time < sequential_time, | ||
| 'Parallel execution should be faster than sequential for batches of large texts' | ||
| end |
There was a problem hiding this comment.
Do you have any idea what the execution time of this code is? In other words: how long does this run (on average)?
There was a problem hiding this comment.
The idea is coming from (and not the idea of but the actual) clock_time.
Minitest has benchmark support, we can use that more directly, I think.
There was a problem hiding this comment.
Should the file end with _test?
| require_relative 'sequential_letter_frequency' | ||
| require_relative 'parallel_letter_frequency' | ||
|
|
||
| class ParallelLetterFrequencyBenchmarkTest < Minitest::Benchmark |
There was a problem hiding this comment.
I don't know if there are other exercises that use benchmarking, but in any case I expect the students to not necessarily know what to do with this file. I would suggest adding some text to the instructions.append.md file and some comments to this file to help the student figure out what they can do with this file.
There was a problem hiding this comment.
The instructions may only apply for those that are working locally, rather than solely on the platform, as well.
But, we can also use https://ruby-doc.org/3.0.6/gems/minitest/Minitest/Benchmark.html as it is included.
ccadden
force-pushed
the
add-parallel-leter-frequency
branch
2 times, most recently
from
dd8e640 to
3c5e737
Compare
ccadden
force-pushed
the
add-parallel-leter-frequency
branch
from
3c5e737 to
db308ad
Compare
|
To expand on the benchmark, might want to link to https://github.com/evanphx/benchmark-ips at least "for further reading". It is the one I will usually use, for the more fixed time comparisons. |
kotp
approved these changes
Mar 3, 2024
exercises/practice/parallel-letter-frequency/.docs/instructions.append.md
Outdated
Show resolved
Hide resolved
exercises/practice/parallel-letter-frequency/.docs/instructions.append.md
Show resolved
Hide resolved
exercises/practice/parallel-letter-frequency/parallel_letter_frequency.rb
Outdated
Show resolved
Hide resolved
ccadden
force-pushed
the
add-parallel-leter-frequency
branch
3 times, most recently
from
276d407 to
ae75090
Compare
Co-authored-by: KOTP <[email protected]>
Co-authored-by: KOTP <[email protected]>
ccadden
force-pushed
the
add-parallel-leter-frequency
branch
from
ae75090 to
b40cc9b
Compare
Co-authored-by: KOTP <[email protected]>
ccadden
force-pushed
the
add-parallel-leter-frequency
branch
from
b40cc9b to
268ea0a
Compare
|
@kotp You merged this, but didn't we also need to install the gem in the test runner? Could you verify that this exercise works via the online editor? |
Getting reports of failure on the platform earlier, timing out(??). But not sure I can see the reason. |
|
I'll have a look. |
|
Ruby 3.0.4 should work (no gem required), and Ruby 3.3 should work as well. Looking at ruby-test-runner repository now. |
|
The tests only reports "failed" in the platform test runner. |
Gem list as above, and the tests for parallel letter test runs fine (locally) and was passing CI, as far as I remember. Locally: |
|
The first problem is that the Ruby test runner runs without networking, which causes: I can fix that by using a fake "internal" network, but we get another error: Thoughts? |
|
I've tried upgrading to Ruby 3.3.0, but to no avail. edit: it does seem to help, because now we get: |
|
Ah, I think I know. It's probably trying to run the benchmark tests, which is shouldn't do in the test runner. I'll work on a fix. |
|
BTW, I think it would be a good idea to add |
|
Working on a fix: exercism/ruby-test-runner#144 |
|
@ErikSchierboom if it's simpler to remove the benchmarking test I'd say go for it. I know other tracks have implemented this with the caveat that we cannot really enforce that you solve it with parallel execution, or that it's faster than a sequential answer. This was an attempt to enforce that. I also noticed the test takes >1s on the GitHub runner compared to the thousandths of seconds for most other spec suites: Let me know and I can open a follow-up to this if needed. Sorry for the headache this morning! |
|
No problem @Mr-sigma , the request to add the name |
diff --git c/exercises/practice/parallel-letter-frequency/parallel_letter_frequency_benchmark_test.rb w/exercises/practice/parallel-letter-frequency/parallel_letter_frequency_benchmark.rb
similarity index 100%
rename from exercises/practice/parallel-letter-frequency/parallel_letter_frequency_benchmark_test.rb
rename to exercises/practice/parallel-letter-frequency/parallel_letter_frequency_benchmark.rb
diff --git c/exercises/practice/parallel-letter-frequency/parallel_letter_frequency_test.rb w/exercises/practice/parallel-letter-frequency/parallel_letter_frequency_test.rb
index 1ca5a992..29ca5379 100644
--- c/exercises/practice/parallel-letter-frequency/parallel_letter_frequency_test.rb
+++ w/exercises/practice/parallel-letter-frequency/parallel_letter_frequency_test.rb
@@ -1,5 +1,6 @@
require 'minitest/autorun'
require_relative 'parallel_letter_frequency'
+require_relative 'parallel_letter_frequency_benchmark' if ENV['BENCHMARK']
# rubocop:disable Layout/SpaceInsideHashLiteralBraces
class ParallelLetterFrequencyTest < Minitest::TestThis patch would restore the benchmark name back to not having the |
I know that there was a message to rename the file to include In practice:
So if we want to document that for the student that would want to run the benchmark. |
|
I've fixed the issue in the test runner, so we don't need to do anything special. What I did find was that the example solution did not pass the tests:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Would love some input about the additional instructions for Ruby parallelism/concurrency specifically!
Forum Post: https://forum.exercism.org/t/add-parallel-letter-frequency-exercise/9948