Add mixin class to replace mandatory hooks #177

casparvl · 2024-09-04T19:24:58Z

This no longer depends on #185
We can add the logging of the version back in as a part of #185

…bles that are needed for those hooks are defined in time, and if not, print a clear message

smoors

thanks a lot for this initial effort toward a mixin class, and for identifying possible issues!

smoors · 2024-09-05T06:55:33Z

eessi/testsuite/eessi_mixin.py

+# Note that I don't think we can do anything about the things set in the class body, such as the parameter's.
+# Maybe we can move those to an __init__ step of the Mixin, even though that is not typically how ReFrame does it anymore?
+# That way, the child class could define it as class variables, and the parent can use it in its __init__ method?


yes, it's unfortunate that there is no support for @run_before('init') methods

smoors · 2024-09-05T06:58:19Z

eessi/testsuite/eessi_mixin.py

+# Hooks from the Mixin class seem to be executed _before_ those of the child class
+# Thus, if the Mixin class needs self.X to be defined in after setup, the child class would have to define it before setup
+# That's a disadvantage and might not always be possible - let's see how far we get


if self.X needs to be defined after setup in the child class, the mixin class can still use it in a @run_before('run') method.

another option is to use @run_after('setup', always_last=True) in the child class which, according to the docs, will ensure those methods will always execute last
https://reframe-hpc.readthedocs.io/en/stable/regression_test_api.html#pipeline-hooks

smoors · 2024-09-05T07:27:30Z

eessi/testsuite/tests/apps/lammps/lammps.py

    scale = parameter(SCALES.keys())
    valid_prog_environs = ['default']
    valid_systems = ['*']
    time_limit = '30m'


this can all go into the mixin class, right? those seem ok default values, which the child can still override if needed?

I've just tried to move that, but it doesn't work for valid_prog_environs, valid_systems and time_limit. I'm getting

WARNING: skipping test file '/gpfs/home4/casparl/EESSI/test-suite/eessi/testsuite/tests/apps/lammps/lammps.py': reframe syntax error: 'time_limit' already defined in class 'EESSI_LAMMPS_base' (rerun with '-v' for more information)

even though I did remove them from the lammp class itself.

eessi/testsuite/eessi_mixin.py

…e dynamic version determined at runtime and store it as a variable in the eessi_mixin class, so it ends up in all tests that are run. This way, you can alwyas see from the run report of reframe which EESSI test-suite version was used

…stalled, try to get it from the git tree

casparvl · 2024-09-19T22:14:25Z

Hm, I went down the rabbit hole of not trying to use hardcoded versions. And then also being able to use them at runtime. It works well from a git checkout. However if I

pip install git+https://github.com/casparvl/test-suite.git@use_mixin_class

The version that is used for some reason is 0.1.dev753+g97e60ab. Although I'm not sure how it is supposed to get any other version: for git, it can check the tags and increment by one. Where should it get it's version (or previous version) if you pip-install out of the blue?

casparvl · 2024-09-20T14:31:15Z

I was being silly. The use_mixin_class is a branch on my fork. Tags are not forked, and thus my fork did not contain any tags.

I now added a tag v0.3.3 to the latest commit in my use_mixin_class branch. Then:

(my_test_venv) [casparl@tcn1 test-suite]$ pip install git+https://github.com/casparvl/test-suite.git@use_mixin_class
Collecting git+https://github.com/casparvl/test-suite.git@use_mixin_class
  Cloning https://github.com/casparvl/test-suite.git (to revision use_mixin_class) to /tmp/pip-req-build-g9hmxril
  Running command git clone -q https://github.com/casparvl/test-suite.git /tmp/pip-req-build-g9hmxril
  Running command git checkout -b use_mixin_class --track origin/use_mixin_class
  Switched to a new branch 'use_mixin_class'
  branch 'use_mixin_class' set up to track 'origin/use_mixin_class'.
  Resolved https://github.com/casparvl/test-suite.git to commit 97e60abb37126bb7165ec66197459f0b08320f88
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
    Preparing wheel metadata ... done
Building wheels for collected packages: eessi-testsuite
  Building wheel for eessi-testsuite (PEP 517) ... done
  Created wheel for eessi-testsuite: filename=eessi_testsuite-0.3.3-py3-none-any.whl size=1854649 sha256=ff462274cc8995e5e592378f90a0c05c65a26dbbc7f441da9a0ea373d11ee0de
  Stored in directory: /tmp/pip-ephem-wheel-cache-k8t31ry2/wheels/65/6f/b7/6fb6ee1323b931899a049e67caaadafd3ee34eb1b811ca39ae
Successfully built eessi-testsuite
Installing collected packages: eessi-testsuite
Successfully installed eessi-testsuite-0.3.3

As expected.

casparvl · 2024-09-20T14:35:56Z

I've also followed the steps for release documented in our own readme, to see if that would also give us the right versioning. It does:

$ python setup.py sdist
...
$ ls -al dist/eessi-testsuite-0.3.3.tar.gz
-rw-r----- 1 casparl casparl 1844717 Sep 20 16:34 dist/eessi-testsuite-0.3.3.tar.gz

Awesome, I think that automatic versioning is working perfectly fine!

casparvl · 2024-09-20T14:37:37Z

Oh, we should probably list setuptools_scm as an (optional?) dependency somewhere, since that will be needed to retreive the version at runtime.

Maybe as an improvement, I should also configure our setup procedure to create a version file, and read from there at runtime as the first option. At least when it is an installed package, that should work. If it's just a git clone, one would still need setuptools_scm to do the runtime version check.

boegel · 2024-09-20T18:25:53Z

Oh, we should probably list setuptools_scm as an (optional?) dependency somewhere, since that will be needed to retreive the version at runtime.

I have some bad experiences with setuptools_scm, if we can avoid it I would stay away from it tbh...

Unless it's a huge win somehow and makes our lives a lot easier.
It may seem to be working out fine, but I've seen it act up once too many times to "trust" it.

Then again, maybe I cry too easily when managing packages... ;)

…t, we're probably in a git-cloned repo, and we resort to using setuptools_scm at runtime to determine the version

…tsuite/__init__.py

…rsion below 8 to stay compatible with python 3.6. Once we don't care about 3.6 compatibility anymore, we can upgrade to setuptools_scm and use version_file instead of write_to

…ks...

casparvl · 2024-09-20T21:38:38Z

Ok, it is a bit silly, since the problem I have been fixing is that we can build the distribution on all python versions in our CI. I don't think that's super important, we really want to be able to install on all version in our CI. Anyway, it works everywhere now. It required pushing the requirement for setuptools_scm to setup.py, because we require logic, as versions will depend on the python version. There is no single version that works for all.

Note that the only thing that is weird is that the version in the CI always seems to be 0.1-dev1. But that's probably due to how things are done in the CI - if I do a python -m build, or a pip install . in my own checkout, the version is completely correct.

… use_mixin_class

…. The current PR now depends on this, as it relies on the eessi/testsuite/__init__.py file to provide a version

casparvl · 2024-09-21T09:13:09Z

I've split of the whole setuptools_scm versioning logic into a separate PR #185

I'd like to get that one merged for the new release (0.3.3). This mixin class will probably take more time, and I don't want to wait with releasing 0.3.3 for this to be finished.

eessi/testsuite/tests/apps/lammps/lammps.py

update mixin class

… use_mixin_class

smoors

works like a charm, this will help a lot getting more people to write testing!

Caspar van Leeuwen and others added 11 commits August 19, 2024 17:37

Merge branch 'main' of github.com:EESSI/test-suite

71e2ea5

Merge branch 'main' of github.com:EESSI/test-suite

fbfe08d

First attempt at using a mixin class

3ae63d8

Expand the number of hooks in the eessi_mixin class. Check that varia…

56d457b

…bles that are needed for those hooks are defined in time, and if not, print a clear message

Comment to make clear how class inheritance works on pipeline hooks

24a0256

Set compute unit in init phase - we already have all info we need there

2bdd84f

Expand comment

3ad1662

Add required_mem_per_node comes from self

119104e

Make sure the required_mem_per_node function returns the value

67d3e4e

More clear SyntaxError message

f096cb9

Added some more suggestions for the future

bee51d2

smoors reviewed Sep 5, 2024

View reviewed changes

smoors reviewed Sep 9, 2024

View reviewed changes

eessi/testsuite/eessi_mixin.py Show resolved Hide resolved

Caspar van Leeuwen added 5 commits September 19, 2024 12:13

Clarify printed message

3ffe7eb

Add memory hook. Make it useable by setting measure_memory_usage

c4a0bcc

Leave a breadcrumb for our future selves

2f84687

First try to get version from installed package. If packate is not in…

97e60ab

…stalled, try to get it from the git tree

Caspar van Leeuwen added 6 commits September 20, 2024 21:29

Make sure scm writes a version file. If the version file doesn't exis…

bf48c2f

…t, we're probably in a git-cloned repo, and we resort to using setuptools_scm at runtime to determine the version

Of course, the extension shouldn't be part of the import...

5095c4f

Import was failing, try something else

3c2f0c6

Now correctly import __version__ in the global namespace of eessi/tes…

04ca0ca

…tsuite/__init__.py

Fix some flake8 errors

28376b7

Fix some more flake8 issues

439c425

Caspar van Leeuwen added 16 commits September 20, 2024 22:47

Remove stray line

95a6d6b

Make flake8 happy again

a79b263

Just stick to write_to for everything, and keep the setuptools_scm ve…

3f0d174

…rsion below 8 to stay compatible with python 3.6. Once we don't care about 3.6 compatibility anymore, we can upgrade to setuptools_scm and use version_file instead of write_to

Remove import that isn't used

430dc10

Fix CI issue for python 3.6

8bbb582

Fix version of setuptools_scm below 8

25f0167

Stick to setuptools_scm < 7 to be compatible with python 3.6

618df8e

Make it < 7 consistently...

e0295ea

Also limit version of packaging used for compatibility with python 3.6

070a549

Make things consistent..

5108a85

let setup.py handle the setuptools_scm conditionally, see if that wor…

c8bf4a6

…ks...

remove stray line

d531e41

Fix flake8

645bb18

Make sure packaging version isn't too new for Python 3.6

ef985d1

Pass list of strings

a6ea23c

Now really fix it

1f1ee2d

Caspar van Leeuwen added 2 commits September 21, 2024 10:54

Merge branch 'use_mixin_class' of github.com:casparvl/test-suite into…

446e4c8

… use_mixin_class

Split off the setuptools_scm versioning into a different pr EESSI#185 …

38ffee1

…. The current PR now depends on this, as it relies on the eessi/testsuite/__init__.py file to provide a version

smoors reviewed Sep 21, 2024

View reviewed changes

eessi/testsuite/tests/apps/lammps/lammps.py Outdated Show resolved Hide resolved

Samuel Moors and others added 5 commits October 6, 2024 15:54

update mixin class

94d16e7

do not override time_limit

b69d1ee

Merge pull request #2 from smoors/pr177

2f6b33a

update mixin class

Merge branch 'use_mixin_class' of github.com:casparvl/test-suite into…

7e66d57

… use_mixin_class

Some cleanup

24951c8

casparvl marked this pull request as ready for review October 9, 2024 09:44

smoors changed the title ~~WIP: Try to use mixin class to replace mandatory hooks~~ Add mixin class to replace mandatory hooks Oct 9, 2024

smoors approved these changes Oct 9, 2024

View reviewed changes

smoors merged commit 548e9b3 into EESSI:main Oct 9, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mixin class to replace mandatory hooks #177

Add mixin class to replace mandatory hooks #177

casparvl commented Sep 4, 2024 •

edited

Loading

smoors left a comment

smoors Sep 5, 2024

smoors Sep 5, 2024

smoors Sep 5, 2024

smoors Sep 5, 2024

casparvl Sep 19, 2024

casparvl commented Sep 19, 2024

casparvl commented Sep 20, 2024

casparvl commented Sep 20, 2024

casparvl commented Sep 20, 2024

boegel commented Sep 20, 2024

casparvl commented Sep 20, 2024 •

edited

Loading

casparvl commented Sep 21, 2024

smoors left a comment

Add mixin class to replace mandatory hooks #177

Add mixin class to replace mandatory hooks #177

Conversation

casparvl commented Sep 4, 2024 • edited Loading

smoors left a comment

Choose a reason for hiding this comment

smoors Sep 5, 2024

Choose a reason for hiding this comment

smoors Sep 5, 2024

Choose a reason for hiding this comment

smoors Sep 5, 2024

Choose a reason for hiding this comment

smoors Sep 5, 2024

Choose a reason for hiding this comment

casparvl Sep 19, 2024

Choose a reason for hiding this comment

casparvl commented Sep 19, 2024

casparvl commented Sep 20, 2024

casparvl commented Sep 20, 2024

casparvl commented Sep 20, 2024

boegel commented Sep 20, 2024

casparvl commented Sep 20, 2024 • edited Loading

casparvl commented Sep 21, 2024

smoors left a comment

Choose a reason for hiding this comment

casparvl commented Sep 4, 2024 •

edited

Loading

casparvl commented Sep 20, 2024 •

edited

Loading