Add optional dependency GSTools-Core

[LSchueler]

I removed all Cython code and replaced it with the GSTools-Core PyPI package v0.1.2.

The Github actions still need to be updated, as the distribution of GSTools has become muuch simpler.

[adamreichold]

One thing that would be nice would be to create a largish regression test suite using the existing implementation and apply that to the new one to ensure equivalent results.

[LSchueler]

Nearly all of the tests done with during the sdist job rely on the Rust implementations. But as you have discovered recently, we still don't cover everything. So, there is some work to do!
The other jobs are only failing due to some import linter problem and that we don't need to build wheels anymore, as GSTools is now a pure Python module.

[adamreichold]

Nearly all of the tests done with during the sdist job rely on the Rust implementations. But as you have discovered recently, we still don't cover everything. So, there is some work to do!

I was pondering something more automated, e.g. using the existing implementation as a model-based test using a coverage-guided fuzzer that would automatically create test cases with the definition of the test cases basically being to yield the same results as the existing implementation. I am just note sure how to integrate the existing implementation, i.e. on a source-level or maybe by just tapping into a fixed binary distribution.

[MuellerSeb]

After a nice conversation with @LSchueler we decided:

• use gstools_core as an optional drop-in replacement and add a switch to control the used backend

  try:
      import gstools_core
  
  except ImportError:
      USE_RUST = False
  else:
      USE_RUST = True
      del gstools_core

• add gstools_core as optional dep. to setup.cfg to install it with pip install gstools[rust]
• solve Refactor Cython routines to have same signatures as rust implementations #216

[adamreichold]

use gstools_core as an optional drop-in replacement and add a switch to control the used backend

Shouldn't this be

try:
    import gstools_core
    USE_RUST = True
except ImportError:
    del gstools_core
    USE_RUST = False

i.e. only if the import succeeds, USE_RUST == True, not unconditionally as the finally block is executed?

[MuellerSeb]

I meant else ... Sorry

[LSchueler]

The Cython code is back in again and GSTools-Core is going to be used if the package is installed and gstools.config.USE_RUST is not manually set to False
I also refactored the Cython code according to issue #216 , which would close it.

[adamreichold]

Shouldn't it be possible to merge those or this is a formatter issue?

[LSchueler]

Yeah... that was isort's idea. It's about as ugly as it gets, right?! :-D And black and isort can't even agree on where to put the # pragma: no cover.
rustfmt is so nice :-)

[MuellerSeb]

What about setting # pragma: no cover behind if config.USE_RUST: once?
Also the # pylint: disable=C0412 is redundant (see line 13) and we could also just add E0401 in line 13.

[LSchueler]

Sorry, I think I was a bit annoyed yesterday by the combination of a false positive coming from pylint and then the isort and black peskiness. I imnproved the situation, but it seems that isort does not like grouped renaming of imports.
I don't want to disable the pylint error E0401 globally, as import errors can be quite a severe problem.

[LSchueler]

For the moment, we don't provide GSTools-Core via conda, which makes it a bit awkward to use it, if GSTools is installed via conda. But I think that's a problem of GSTools-Core and as the Rust implementations are labeled as experimental anyway, I think this is ready to be merged.

[MuellerSeb]

This is really cool! I got some ideas for improvement, maybe you like them. And what about a routine to set the env-var for the number of cores for the rust parallel runs in gstools.config?

[MuellerSeb]

What about a little function in gstools.config to set this environment variable?

[adamreichold]

One argument against that is that this variable will affect all Rust software using Rayon, not just GSTools-Core's backend.

If we do provide a programmatic interface, we should probably make the Core initialize a dedicated thread pool and make this function affect only this thread pool?

[LSchueler]

@adamreichold raises a fair point there. But that env. variable would only exist during the shell session from which GSTools is run (I don't have enough experience with Windows to know how it handles env. variables).
Did I understand it correctly, that the number of threads used by Rayon can only be set once at the start and doing it a second time results in an error? I'm not if this is worth the effort.
Another alternative would be to capture the env. variable at the start of GSTools and set it back to that value when exiting, but that doesn't sound very elegant.
I personally would just keep the hint in the readme for the moment. But if you have stronger opinions than I do, I'd be happy to implement your preferred solution.

[adamreichold]

Did I understand it correctly, that the number of threads used by Rayon can only be set once at the start and doing it a second time results in an error? I'm not if this is worth the effort.

Setting the environment variable will affect Rayon's so-called global thread pool which is initialized once, either on-demand when the first Rayon task is spawned or explicitly be the application. Hence, changing the environment variable after a first computation using Rayon was run has no effect.

This why I suggested making the GSTools-Core module store an Option<ThreadPool> which would be affected (meaning re-initialized using a given number of threads) by the function from gstools.config. If the option is set, that thread pool is used via ThreadPool::install, otherwise GSTools-Core falls back to the global thread pool.

I personally would just keep the hint in the readme for the moment.

Personally, I would say that online control over parallelism is a somewhat orthogonal issue and would best be served via a follow-up PR / issue.

[MuellerSeb]

What about setting # pragma: no cover behind if config.USE_RUST: once?
Also the # pylint: disable=C0412 is redundant (see line 13) and we could also just add E0401 in line 13.

[LSchueler]

I think an import error (E0401) should only be disabled locally.

[MuellerSeb]

LGTM