paris-saclay-cds / ramp-workflow

Toolkit for building predictive workflows on top of pydata (pandas, scikit-learn, pytorch, keras, etc.).

https://paris-saclay-cds.github.io/ramp-docs/

BSD 3-Clause "New" or "Revised" License

68 stars 43 forks source link

[MRG] Imports and cloudpickle #237

Closed albertcthomas closed 4 years ago

albertcthomas commented 4 years ago

Fixes #232

[x] Add a comment to import_module_from_source explaining why we don't add the module to sys.modules.
[x] Add a test checking that cloudpickle works on an object that can be pickled with cloudpickle. Note that running ramp-test with --pickle does not really test this because the pickling errors are caught.

albertcthomas commented 4 years ago

~Hm the failing tests (which are also failing on master apparently) might be my fault because of not catching all exceptions when pickling fails in #233. There is a TypeError and not a PicklingError. Strange that it worked at the time of the PR.~ No need after CI was fixed, see PR #240.

albertcthomas commented 4 years ago

Now we have a failing test because pip install keras in the testing-requirement.txt installs the last version Keras 2.4 which requires tensorflow 2.2, whereas tensorflow 2.0.1 is specified in testing-requirement.txt.

agramfort commented 4 years ago

update testing-requirement.txt. ?

albertcthomas commented 4 years ago

Done in PR #240

codecov[bot] commented 4 years ago

Codecov Report

Merging #237 into master will increase coverage by 0.04%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
+ Coverage   81.70%   81.75%   +0.04%     
==========================================
  Files         133      133              
  Lines        4909     4921      +12     
==========================================
+ Hits         4011     4023      +12     
  Misses        898      898

Impacted Files	Coverage Δ
rampwf/utils/importing.py	`100.00% <ø> (ø)`
rampwf/tests/test_kits.py	`95.55% <100.00%> (+1.61%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4d5c902...67d1292. Read the comment docs.

albertcthomas commented 4 years ago

CIs are green after rebasing on master. No need to change the error handling when pickle fails for now.

albertcthomas commented 4 years ago

@glemaitre maybe you would like to have a quick look at this PR if you have some time.