New style checks #264

vkarak · 2018-04-22T21:33:08Z

This PR provides two things:

A major refactoring of the host resources code. The unit tests were also substantially refactored.
A new simplified syntax for the ReFrame test files that eliminates all the boilerplate code.

Host resources and unit tests refactoring

The refactoring of the resources code is based on the following concept:

All the information about the system that the framework is currently running on is encapsulated in a global unique runtime object of type RuntimeContext. This information includes the current host system, the current modules system and the directory resources management (ex ResourcesManager).
The system and resources arguments that used to be passed through to the RegressionTests are removed. This information is now accessible inside the RegressionTest by the runtime. This change is transparent to the current regression tests.
The runtime may be initialised only once in the lifetime of the whole ReFrame. Unit tests though may temporarily switch the runtime, but this functionality is not intended to used by the framework. Access to to the runtime is given by calling the reframe.core.runtime.runtime() function, which returns a RuntimeContext object, from which access to the host system, modules system and the resources manager is given.
Unit tests that require a native configuration file (e.g., those that need to submit jobs) are reorganised substantially, homogenised and simplified. The unit tests driver program, test_reframe.py, initialises the framework's runtime based on a special settings file unittests/resources/settings.py. This file defines a generic system to allow the unit tests to run on any machine and fake one (ex TEST_SITE_CONFIG in fixtures) for testing the framework's behaviour with site-specific settings.
The test_reframe.py driver adds an option, --rfm-user-config, for specifying a custom user configuration file. Contrary to the past, these settings DO NOT replace globally the ReFrame configuration during the unit tests. Instead, only the unit tests that need a specific user configuration (e.g., SLURM tests) temporarily change to the user configuration using the @fixtures.switch_to_user_runtime. These tests then do the necessary setup and check if they can continue or be skipped.

Other changes brought by this PR (related to unit tests and host resources):

The init_modules_system() and get_modules_system() methods are removed. Instead a modules system is created by calling the ModulesSystem.create() method. To retrieve the current modules system, one has to first retrieve the runtime and then access it from there.
The settings are not stored in the framework internally. It would now only be needed for retrieving the logging settings, but this is not the case, so for simplicity, only the load_settings() (ex load_from_file()) method is retained. Accessing anything related to the global configuration may only be done through the global RuntimeContext.
All the import statements of the SiteConfiguration class were moved inside its load_from_dict() method to avoid cyclic dependencies at import time. Since the this class creates several core components of the framework it needs to be as independent as possible at the import time. This is called by the runtime to set itself up, but other basic components of the framework may need the runtime services to fulfil their operations.
The SiteConfiguration class moved to the core.
The sanitisation of the system, partition and environments names in RegressionTest._setup_paths() is now replaced by setting up the appropriate validation of the corresponding attributes in the respective classes. To achieve this an ExtendedAlphanumericField was added that accepts - as valid characters apart from those typically accepted by an alphanumeric field.
The System class is now immutable. All the user-supplied directory prefixes are set in the HostResources. This class uses properties to compute dynamically the actual prefixes (e.g., using or not timestamps)
The generate_test_config of the unit tests is now removed. Log files and other resources pertaining to the unit tests ReFrame configuration have a special rfm_ prefix.
The force_remove_file() function moved to utility.os_ext.
The unit tests for the SiteConfiguration are added again, after accidentally being removed in a previous PR.
A new field is added to automatically convert an assigned path to its absolute value.

New-style checks

The boilerplate code for calling the RegressionTest constructor is no more required; the base constructor may now be called without any arguments.
The _get_checks() method is no more required. The test are registered with class decorators.
The basic classes and decorators are now exported at the top-level reframe module for convenience.

Other improvement and fixes brought by this PR:

Use our module load mechanism only if an absolute path is passed. Otherwise use importlib.import_module, which also takes care of the PEP420 namespace packages.
To decouple the frontend code from the CLI further, a new SystemAutodetectionError is raised in cases of system autodetection failure.
All the ReFrame test files used for unit tests are now moved under a new directory named checks/ in unittests/resources. This was done to allow to have separate directories for unlisted tests, i.e., that need not to be listed and counted by the loader's and policies' unit tests.
The API of the the ModulesSystem class is now exposed to and documented for the users.

Still in progress

~~The reframe.core.decorators is a bit obsolete now.~~
~~The new style checks are still to come, but it should now be easy to be implemented.~~
~~CI script needs to be adapted to use the --rfm-user-config option.~~
~~A bit of code cleanup.~~
~~Update documentation.~~

Fixes #51.
Fixes #70.

- No more `ResourcesManager` - A global runtime (`RuntimeContext`) is introduced, which holds all the information about the system the framework is currently running. This information includes the current host system and its configuration, the associated modules system and the resources of the framework (ex `ResourcesManager`). Anybody who wants to access any of this information, he may do so by calling `runtime()` to retrieve the `RuntimeContext` instance. - Unit tests are considerably refactored to match this redesign. The `test_reframe.py` initialises a runtime (either a user-specified one or the `generic` one from some predefined unit-test-specific settings. Also, functionality to temporarily switch to a different runtime (not possible normally) is offered to the unit tests for special test cases. Job scheduler unit tests became a bit more generic allowing easier testing of new backends.

- With Python 3.5 - On systems without Tmod

Both for unit tests and the normal ReFrame invocations.

…into feature/new-style-checks

vkarak · 2018-04-23T11:56:56Z

@jenkins-cscs retry all

omlins

LGTM so far (I found only 2 little things)

omlins · 2018-04-24T10:17:07Z

reframe/core/runtime.py

+
+    @property
+    def partitions(self):
+        """Get all the partitions of this system.


In agreement with the way you did the comments for name, descr and resourcesdir, I would note here instead:

"""All the partitions of this system.

or

"""The partitions of this system.

omlins · 2018-04-24T10:36:16Z

unittests/fixtures.py

-def partition_with_scheduler(name, skip_partitions=['kesch:pn']):
-    """Retrieve a partition from the current system whose registered name is
+def partition_with_scheduler(name=None, skip_partitions=['kesch:pn']):
+    """Retrieve a natice system partition whose scheduler is registered with


natice -> native

- The boilerplate code for calling the `RegressionTest` constructor is no more required; the base constructor may now be called without any arguments. - The `_get_checks()` method is no more required. The test are registered with class decorators. - The basic classes and decorators are now exported at the top-level `reframe` module for convenience. Other improvement and fixes brought by this commit: - Use our module load mechanism only if an absolute path is passed. Otherwise use `importlib.import_module`, which also takes care of the PEP420 namespace packages. - To decouple the frontend code from the CLI further, a new `SystemAutodetectionError` is raised in cases of system autodetection failure. - All the ReFrame test files used for unit tests are now moved under a new directory named `checks/` in `unittests/resources`. This was done to allow to have separate directories for unlisted tests, i.e., that need not to be listed and counted by the loader's and policies' unit tests.

victorusu

This is definitely a long change.
I took forever reviewing it today.

I am excited with the use of the visitor pattern for the RegressionCheckValidator.
I can see it being used for other things too.

The new syntax is awesome!
Great job!

victorusu · 2018-04-30T14:12:46Z

reframe/frontend/cli.py

    # Load configuration
    try:
-        settings = config.load_from_file(options.config_file)
+        settings = config.load_settings(options.config_file)


Let me a bit picky...
Can we stick with the load_from_file? Or have it load_settings_from_file? Or just settings_from_file or from_file?
I think it is important to stick to the "from file" part. It makes easier to understand the code

I didn't want to use the load_from_file, cos this makes more sense as a method of class. Nonetheless, I will change it to settings_from_file() or load_settings_from_file().

victorusu · 2018-04-30T14:20:51Z

reframe/utility/__init__.py

+    if not s:
+        return ''
+
+    return re.sub(r'([a-z])([A-Z])', r'\1_\2', s).lower()


Is it enough to just call s.lower()?

What do you mean?

@victorusu What do you mean here?

We finally don't use this function. I have kept it though, because it's a perhaps useful utility.

omlins

I think the automatic generation of test.name needs to be revised.

omlins · 2018-05-11T10:57:43Z

unittests/test_pipeline.py

+                super().__init__(1, 2)
+
+        test = MyTest()
+        self.assertEqual('my_test', test.name)


As we enforce since version 2.12 that the names of the loaded tests must be unique (https://eth-cscs.github.io/reframe/running.html?highlight=unique#discovery-of-regression-tests), I have the following doubt. Don't we get non-unique names e.g. in the following scenario?

class BaseMatrixVectorTest(RegressionTest): def __init__(self): ... ... class SerialTest(BaseMatrixVectorTest): def __init__(self): ... ...

...and (in another file):

class BaseStencilTest(RegressionTest): def __init__(self): ... ... class SerialTest(BaseStencilTest): def __init__(self): ... ...

We get in both cases test.name equal to serial_test, right? If I am not overseeing something and it is like that, then I think this must be avoided, because it is not an unlikely scenario. One solution could be to prefix the generated names additionally with the name(s) of the super class(es).

Personally, I can't see this as a blocking issue for this PR. The class name is a guess and a convenience; the users may always set their own name if a conflict occurs or if they don't trust the auto-generation mechanism. Whichever heuristic we had with the name, there would always be a case to fool it. In your scenario, it could be easily fooled if the same class names were also present at another file. Then, we end up having to include the name of the file. Oops, this is not enough either! Because, we might have two files with the same class structure in different directories and so on an so forth. We would end with too long, unusable test names, forcing the users to use their own... After all, the conflict detection mechanism is still valid, since it checks the names during loading the test.

I totally agree that the test name generation does not need to be bullet proof; it only needs to catch the common scenarios (as we have discussed already some days ago off-line). Now, how common the above scenario is depends very much on the programming habits of the users.

Besides the uniqueness issue, the test name should be most meaningful but not too long as you pointed out. One could maybe use somehow the name of the direct superclass (if it is defined by the user) to make the test names more meaningful. However, this could already make the name too long...

In any case this issues are certainly not blocking as you say. You may merge it as is and we may see later if the automatic test name generation can still be improved...

I think it would be good to explain in the documentation how the names are generated such that the users may make good usage of it.

To fix this, I'm gonna use the fully qualified name of the class from the __qualname__ attribute.

@omlins Prefixing with the module name (or anything derived from the filesystem) is not a good idea. The __qualname__ does not do that deliberately. Have a look here. I suggest getting along with just the qualified name (which covers also nested classes compared to __name__) and if a conflict occurs, then the user may define its own name.

vkarak · 2018-05-23T09:56:46Z

@omlins Can you review again and possibly approve this PR?

vkarak · 2018-05-23T11:12:01Z

@jenkins-cscs retry all

vkarak · 2018-05-23T12:54:13Z

Raised the priority of this PR. We need to merge asap, cos it's blocking everything else in the sprint.

vkarak · 2018-05-23T15:39:43Z

@omlins Thanks for approving. I am still not entirely sure whether it's better to prefix the the automatically generated test name with the module name or not, but let's leave it open for discussion internally.

vkarak added request for enhancement prio: important enhancement frontend labels Apr 22, 2018

vkarak added this to the ReFrame sprint 2018w16 milestone Apr 22, 2018

vkarak self-assigned this Apr 22, 2018

vkarak requested review from omlins, teojgo and victorusu April 22, 2018 21:33

Vasileios Karakasis added 3 commits April 23, 2018 09:29

Fix unit tests failures.

53012dc

- With Python 3.5 - On systems without Tmod

Fix CI script to use the config options

2e6ba85

Both for unit tests and the normal ReFrame invocations.

Merge branch 'feature/new-style-checks' of github.com:vkarak/reframe …

c730921

…into feature/new-style-checks

omlins suggested changes Apr 24, 2018

View reviewed changes

vkarak added prio: normal and removed prio: important labels Apr 27, 2018

vkarak modified the milestones: ReFrame sprint 2018w16, Upcoming sprint Apr 27, 2018

victorusu approved these changes Apr 30, 2018

View reviewed changes

vkarak mentioned this pull request May 2, 2018

Provide a way to tests to specify the ReFrame version that they require #166

Closed

Vasileios Karakasis added 4 commits May 2, 2018 18:07

Merge branch 'master' into feature/new-style-checks

09b7425

Fix unit tests after merge with master

fd276c8

Merge branch 'master' into feature/new-style-checks

b92550f

Rename checks_newstyle/ to checks_unlisted/

d53596e

vkarak changed the title ~~WIP: New style checks~~ [WIP] New style checks May 3, 2018

teojgo approved these changes May 4, 2018

View reviewed changes

omlins suggested changes May 11, 2018

View reviewed changes

Merge branch 'master' into feature/new-style-checks

75dfa85

Vasileios Karakasis added 2 commits May 17, 2018 16:39

Use __qualname__ for automatic test naming.

62374ef

Update documentation for the new test syntax.

fd9b490

vkarak changed the title ~~[WIP] New style checks~~ New style checks May 23, 2018

vkarak added prio: important and removed prio: normal labels May 23, 2018

omlins approved these changes May 23, 2018

View reviewed changes

Documentation fixes

8c7df51

vkarak merged commit b5ab990 into reframe-hpc:master May 23, 2018

vkarak deleted the feature/new-style-checks branch May 23, 2018 16:28

vkarak mentioned this pull request May 25, 2018

Fix test listing in documentation #292

Closed

vkarak mentioned this pull request Jun 6, 2018

--timestamp command-line option is ignored #315

Closed

New style checks #264

New style checks #264

Uh oh!

Conversation

vkarak commented Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Host resources and unit tests refactoring

Other changes brought by this PR (related to unit tests and host resources):

New-style checks

Other improvement and fixes brought by this PR:

Still in progress

Uh oh!

vkarak commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omlins left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victorusu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omlins left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omlins May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkarak commented May 23, 2018

Uh oh!

vkarak commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vkarak commented May 23, 2018

Uh oh!

vkarak commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vkarak commented Apr 22, 2018 •

edited

Loading

vkarak commented Apr 23, 2018 •

edited

Loading

omlins May 11, 2018 •

edited

Loading

vkarak commented May 23, 2018 •

edited

Loading