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 script for generating bigpoly test-data #8470

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonathan-eq
Copy link
Contributor

Approach
✏️

(Screenshot of new behavior in GUI if applicable)

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure tests pass locally (after every commit!)

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.67%. Comparing base (acfc8c9) to head (2b98021).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8470      +/-   ##
==========================================
- Coverage   90.82%   90.67%   -0.16%     
==========================================
  Files         348      346       -2     
  Lines       21143    21022     -121     
==========================================
- Hits        19204    19062     -142     
- Misses       1939     1960      +21     
Flag Coverage Δ
gui-tests 75.92% <ø> (-0.23%) ⬇️
integration-tests 53.97% <ø> (-0.22%) ⬇️
unit-tests 68.69% <ø> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonathan-eq jonathan-eq force-pushed the add-bigpoly-script branch 2 times, most recently from 8a6909a to 57cf826 Compare August 15, 2024 09:58
@@ -34,3 +34,6 @@ src/ert/shared/version.py
/compile_commands.json
*_pb2.py
*_pb2.pyi
/test-data/bigpoly/*
!/test-data/bigpoly/make_bigpoly.sh
!/test-data/bigpoly/generate_eclsum.py
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need these lines here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should add them for normal poly_example too. It is very easy to commit logs and poly_out by mistake

@berland
Copy link
Contributor

berland commented Aug 16, 2024

Should we have a PR in gkomodo-releases that demonstrates that it is able to build on this PR?

It should get the grdecl script from Ert and generate the remaining queue system tests based on the files dumped by this script.

@xjules
Copy link
Contributor

xjules commented Aug 16, 2024

Should we have a PR in gkomodo-releases that demonstrates that it is able to build on this PR?

It should get the grdecl script from Ert and generate the remaining queue system tests based on the files dumped by this script.

Yes, I wanted to suggest the same. Also to have a small description how to run / build big poly or is it so simple to run the make_bigpoly should be enough?

@jonathan-eq
Copy link
Contributor Author

Should we have a PR in gkomodo-releases that demonstrates that it is able to build on this PR?
It should get the grdecl script from Ert and generate the remaining queue system tests based on the files dumped by this script.

Yes, I wanted to suggest the same. Also to have a small description how to run / build big poly or is it so simple to run the make_bigpoly should be enough?

I want to add a file modify_bigpoly.sh in komodo-releases/
The script will checkout the bigpoly script from ert, run it, then append the other queue_options for cluster

@xjules
Copy link
Contributor

xjules commented Aug 16, 2024

After giving big-poly some thoughts I've realized that this case / test should remain only in komodo IMHO. Semantically it only serves as a testing detail related to our production environment and as such it does not provide any value to potential users of ert. Any thoughts @sondreso @berland

@jonathan-eq
Copy link
Contributor Author

After giving big-poly some thoughts I've realized that this case / test should remain only in komodo IMHO. Semantically it only serves as a testing detail related to our production environment and as such it does not provide any value to potential users of ert. Any thoughts @sondreso @berland

When refactoring snapshots, it was very useful to have a heavy run, so we had to manually clone over bigpoly from komodo-releases. This was more difficult than it had to be, and most of the bigpoly files created by the script are not for running on cluster anyways.

Instead of manually cloning the script from komodo-releases and removing the cluster side of it, i think it is better to have the essential part of the script in Ert, and a smaller script for bigpoly_cluster in komodo.

@xjules
Copy link
Contributor

xjules commented Aug 19, 2024

Suggestion: Let's create a new issue, which would aim to have such a case in test-data, where we should give it a better name (than big-poly) and make a readme, which describes utilization of such a case; ie. stress-testing the messaging system and internationalization.

@berland
Copy link
Contributor

berland commented Aug 19, 2024

It probably belongs in tests/performance_tests as a pytest test.

@xjules
Copy link
Contributor

xjules commented Sep 2, 2024

It probably belongs in tests/performance_tests as a pytest test.

do you mean to move the entire big-poly there?

@berland
Copy link
Contributor

berland commented Sep 2, 2024

It probably belongs in tests/performance_tests as a pytest test.
do you mean to move the entire big-poly there?

Probably a more carefully written test setup that in some way stresses the message system without touching implementation details, with and without some mocked cpu-intensive internalization.

@jonathan-eq
Copy link
Contributor Author

It probably belongs in tests/performance_tests as a pytest test.

I don't think so. I mainly use bigpoly for manually testing the GUI. If we want a pytest performance test for it, we can create a fixture and use it the same way we use copy_poly_example. It is easier to do it this way than the other way around.

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