Closed jonastim closed 1 year ago
The test fails because the VW package is missing. Should we add VW to the test dependencies or should I remove the test? I could potentially mock the VW instance out but I am not sure how useful the test still is then.
tldr; Nice. No that is an easy fix. I can correct the test this afternoon. (Or you can if you want it sooner based on text below)
Full Explanation:
The wofklow actually runs all tests twice, once with all optional dependencies installed and once with no optional dependencies installed. That way we can make sure coba works with both the minimum dependencies and maximum dependencies.
For the test, you can put a check before the unit test to skip it if the required package isn't installed and the test will be skipped when the minimum dependencies are installed and executed when all packages are installed. For an example of this you can look in the vowpal learner unit tests and search for skipUnless
.
Additionally, in coba.utilities we have a static class called PackageChecker which we use to print out a friendly message when users try to use functionality with more advanced dependencies. So in init I might put something like this,
if 'ope_loss' in record:
PackageChecker.vowpalwabbit('SimpleEvaluation.__init__')
That will make sure VW is installed and print out a friendly error message showing how to install VW if it isn't.
My thought process with this pattern was that we can now use all the dependencies we want in coba and only require the user to install them if they really need them. For example the ClassEnvironmentInfo task depends on sklearn but 99% of people will probably never use it so it doesn't make sense to require it. Maybe the slow trickle of dependencies is annoying to new users, I don't know, but for me personally I love packages with very few explicit dependencies.
Hey also, as we merge this stuff in if you ever want me to cut a release so it is easier to deploy to your team just let me know.
Ah great, that makes a lot of sense! Just pushed a commit with your suggestions. I am also a fan of keeping dependencies minimal. You might want to look into adding them as optionals, either for specific functionality or testing.
In our RL library we have for example this section in the setup.cfg
[options.extras_require]
vowpalwabbit =
vowpalwabbit
featureimportance =
shap
matplotlib
and people can consume it by pip install rl-lib[featureimportance]
.
Hey also, as we merge this stuff in if you ever want me to cut a release so it is easier to deploy to your team just let me know.
That would be great! I am planning to look at two more things this week that might or might not require some tweaks to Coba:
Depending on how much effort it is to cut a release you might want to wait on those.
Oh, direct support for optional dependencies in setup tools is hot.
I actually didn't know about that functionality. It's been on my todo list to to migrate coba to the new toml setup-tools format. This is a good motivator to finally do it. Right now all this is managed with an environment file in the test directory that contains all "optional" dependencies.
Releasing only takes like 10 minutes. I use coba on a daily basis so sometimes I just let changes accumulate for a while to simply get more "integration" testing before I release. Two weeks ago I rewrote the multiprocessing layer (previously it could get into irrecoverable states when a KeyboardInterrupt was sent) so I've just been waiting until I felt more confident in the changes before releasing.
Regarding mini batch updates, I'm not sure if you've seen it or if it is what you need, but there is some support for batching built into coba. It was added for learners built in pytorch, but all learners in coba should currently work with batches. All that changes is each "interaction" in Result.interactions now represents a batch.
@jonastim FYI, I just released. The latest version is 6.2.6.
Ability to record VW's OPE loss metric for evaluating model performance