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

Is the chapel_unit_test_linux CI Step Necessary? #1550

Closed
Ethan-DeBandi99 opened this issue Jul 1, 2022 · 4 comments · Fixed by #1628
Closed

Is the chapel_unit_test_linux CI Step Necessary? #1550

Ethan-DeBandi99 opened this issue Jul 1, 2022 · 4 comments · Fixed by #1628
Assignees
Labels
question Further information is requested

Comments

@Ethan-DeBandi99
Copy link
Contributor

This issue is prompted by PR #1542. After some discussion with @joshmarshall1 and @pierce314159, it seems that most, if not all, of the functionality covered here is also covered by the python testing. With how long the CI takes to run, I am not sure it makes sense to duplicate these efforts because each step takes 30+ seconds to complete.

@reuster986, @mhmerrill, @bradcray, @ronawho, @bmcdonald3 - does anyone have objects to doing this (provided we verify that all functionality covered by the chapel tests are covered by the python as well)?

@ronawho
Copy link
Contributor

ronawho commented Jul 1, 2022

At least for me, the appeal of the standalone unit tests is the ability to test some features without compiling all of Arkouda. This may be less important now with the addition of modular builds and --saveUsedModules but I personally still use it frequently for test/UnitTestSort.chpl when I'm tuning sort performance.

I'm not opposed to removing these tests still, but I'll almost certainly use something like a few of them for perf tuning from time to time. As far as correctness testing that's only in these tests -- I think some of the parquet testing is only done there but Ben is the expert there (and he's out till the 10th). There are also some interesting bit pattens in UnitTestSort that I don't think are fully covered in python tests.

@Ethan-DeBandi99
Copy link
Contributor Author

I don't think our intention was to totally get rid of the tests, just to remove the CI step. As for Parquet, we now require pyarrow, so we can test a lot of that from python as well. Additionally, I am updating most of the HDF5 and Parquet functionality currently, so I will be adding more testing in python.

@ronawho
Copy link
Contributor

ronawho commented Jul 1, 2022

I don't think our intention was to totally get rid of the tests, just to remove the CI step.

I see. That's fine with me. The tests may suffer from bitrot if the things they call get changed, but it seems reasonable to just have to update them when somebody needs them. If you take that approach I'd probably recommend getting rid of some tests that don't have much value anymore.

As for Parquet, we now require pyarrow, so we can test a lot of that from python as well. Additionally, I am updating most of the HDF5 and Parquet functionality currently, so I will be adding more testing in python.

There was something about doing this in the chapel test that was easier than doing it in python tests or maybe it avoided bringing in some other dependency. I really don't remember the details but Ben should. (And it's possible my information is outdated now and this was just a limitation as we were initially adding support.)

@stress-tess
Copy link
Member

i think removing them from the CI is a good idea! We just gotta take a bit of time to look over the chapel tests and add any functionality only covered by them to the python tests first

I don't personally ever use the chapel test in my development process. I think keeping them around but not running them in the CI seems like a good way to go. The tradeoff of shorter CI times for someone having to do a bit of updating to use those tests seems worth it to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
4 participants