PetrochukM / PyTorch-NLP

Basic Utilities for PyTorch Natural Language Processing (NLP)
https://pytorchnlp.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2.21k stars 258 forks source link

remove lambdas for pickle #42

Closed benjamin-work closed 6 years ago

benjamin-work commented 6 years ago

lambda cannot be pickled, therefore it is better to not use it as attribute. Even though there are alternatives to pickle, some libraries use pickle internally, which is why it's better to support it.

Tests were added to all samplers and encoders for whether objects can be pickled.

codecov-io commented 6 years ago

Codecov Report

Merging #42 into master will increase coverage by 0.01%. The diff coverage is 96.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   94.55%   94.57%   +0.01%     
==========================================
  Files          56       56              
  Lines        1579     1603      +24     
==========================================
+ Hits         1493     1516      +23     
- Misses         86       87       +1
Impacted Files Coverage Δ
torchnlp/text_encoders/delimiter_encoder.py 94.11% <100%> (+1.26%) :arrow_up:
torchnlp/text_encoders/whitespace_encoder.py 90.9% <100%> (+2.02%) :arrow_up:
torchnlp/text_encoders/spacy_encoder.py 83.33% <100%> (+2.38%) :arrow_up:
torchnlp/samplers/sorted_sampler.py 100% <100%> (ø) :arrow_up:
torchnlp/text_encoders/identity_encoder.py 93.33% <100%> (+1.02%) :arrow_up:
torchnlp/samplers/noisy_sorted_sampler.py 100% <100%> (ø) :arrow_up:
torchnlp/samplers/noisy_sorted_batch_sampler.py 100% <100%> (ø) :arrow_up:
torchnlp/text_encoders/character_encoder.py 90.9% <100%> (+2.02%) :arrow_up:
torchnlp/text_encoders/static_tokenizer_encoder.py 97.05% <100%> (+0.18%) :arrow_up:
torchnlp/samplers/bucket_batch_sampler.py 95.34% <75%> (-2.09%) :arrow_down:

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 aa23d5f...644d105. Read the comment docs.

benjamin-work commented 6 years ago

I should add that I couldn't run all tests because firewall rules blocked downloads of external data*. But as a I see it, those tests shouldn't be affected by my changes.

*PS: Maybe all tests that depend on external data could get a pytest.mark.skipif to skip them if external data is unavailable.

PetrochukM commented 6 years ago

@benjamin-work The tests do not download any resources, they run: urllib.request.urlopen(url).getcode() == 200

I do not know how to check beforehand if this will work. Do you know?

PetrochukM commented 6 years ago

Sent a commit to your branch fixing the code coverage issues!

BenjaminBossan commented 6 years ago

I do not know how to check beforehand if this will work

Something like this might work (not tested):

# in conftest.py
@pytest.fixture(scope='session')
def connection_possible():
    return urllib.request.urlopen(some_url).getcode() == 200

# in actual test module
@pytest.mark.skipif(not connection_possible, reason='no internet connection')
def test_it():
    ...