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

Fixes SEGFAULT in integration_tests/elemental_01.py #2451

Closed
wants to merge 5 commits into from

Conversation

Kishan-Ved
Copy link
Contributor

On running /integration_tests/run_tests.py, a SEGFAULT error occurs and the test fails:

The following tests FAILED:
        159 - elemental_01 (SEGFAULT)
Errors while running CTest
Command failed: ctest -j8 --output-on-failure

Upon careful experimentation, I found that this is due to lines 106 and 107 in file elemental_01.py, where creating very large arrays of f64[256,64,16] causes memory overflow. The lines causing the issue are:

    arraynd: f64[256, 64, 16] = empty((256, 64, 16), dtype=float64)
    sinnd: f64[256, 64, 16] = empty((256, 64, 16), dtype=float64)

I have changed the datatype to f32 where needed. This allows the required large arrays can be created and tested. Alternatively, we can increase the memory limit of the test.

@Kishan-Ved
Copy link
Contributor Author

Someone please help with failing checks, here's some information from a failing check: LPython CI / LPython CI (3.10, ubuntu-latest) (pull_request):

compiler_tester.tester.RunException: Testing with reference output failed.
../integration_tests/elemental_01.py * asr
The JSON metadata differs against reference results
Reference JSON: tests/reference/asr-elemental_01-b58df26.json
Output JSON:    tests/output/asr-elemental_01-b58df26.json
Omitting 9 identical items
Differing items:
{'infile_hash': '1af419aefa306307a6b0660fb3fb28ac707dd53c2a8dddf16c07e412'} != {'infile_hash': '1d1eb8ce26df5817c1e474e4f69b0b96e53df362a31f1b722efaadf0'}
{'stdout_hash': 'b7d28110f91ceddb10a422c7b3a1fe2f737a2f147b6fa033160cbd09'} != {'stdout_hash': 'f6657ff256291caa10a0681ae7c5ad89f5c103725e44318d42b1445e'}
Diff against: tests/reference/asr-elemental_01-b58df26.stdout

Do I need to change some hash?

@czgdp1807
Copy link
Collaborator

These tests don't fail on CI though. Check main branch. And arrays are created on heap by default.

@Kishan-Ved
Copy link
Contributor Author

Sorry, I'm unable to understand. I cloned the main branch and followed the instructions given here: https://github.com/lcompilers/lpython?tab=readme-ov-file#tests
The error I mentioned above appeared and I fixed it.

@Thirumalai-Shaktivel
Copy link
Collaborator

Which OS, do you use?
Yup, it doesn't fail for me as well (on macOS)!

@Kishan-Ved
Copy link
Contributor Author

I use Ubuntu 22.04.3 LTS

@Kishan-Ved
Copy link
Contributor Author

Any updates on this?

@certik
Copy link
Contributor

certik commented Jan 4, 2024

If we are having memory overflow, then let's create smaller arrays. I would however keep the original types, to ensure they keep working.

@Kishan-Ved
Copy link
Contributor Author

If we are having memory overflow, then let's create smaller arrays. I would however keep the original types, to ensure they keep working.

Should I change the test to one with smaller arrays and the original datatype as suggested?

@Kishan-Ved Kishan-Ved closed this Jan 4, 2024
@Kishan-Ved
Copy link
Contributor Author

Kishan-Ved commented Jan 4, 2024

I think I accidentally closed this.

@Kishan-Ved Kishan-Ved reopened this Jan 4, 2024
@certik
Copy link
Contributor

certik commented Jan 4, 2024

Should I change the test to one with smaller arrays and the original datatype as suggested?

Yes, I would do that.

Make sure it works on your machine. If CI passes, then we can merge it.

@Kishan-Ved
Copy link
Contributor Author

This change gets rid of the segfault, but ./runtests.py (in lpython directory) gives me this hash error, please let me know how the hashes are updated / how the reference files are updated.

@Kishan-Ved
Copy link
Contributor Author

Please check this PR: #2452
I did my work in a new branch and the default settings in my Github created a new pr to the base branch. I'm new to open source and hence such an issue. I'll be careful next time. We can merge that PR and close this.

Also, I noticed that the instructions given in README for Linux don't include ./run_tests.py -u which is necessary to update the reference tests.

@Kishan-Ved
Copy link
Contributor Author

Closing this PR as #2452 has been merged. Thanks!

@Kishan-Ved Kishan-Ved closed this Jan 6, 2024
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.

4 participants