Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow notebook env #11

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,7 @@ jobs:
architecture: 'x64'
- name: Install pytest
run: pip install -U pytest
- name: Install dependencies
run: pip install -r requirements.txt
- name: Run tests
run: make pytest
13 changes: 13 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,19 @@ Check out detailed example below

![subdir](img/subdir_demo.png)

### 2.3 Testing functions from notebook
If you want to pass the functions from the notebook to the test case you need to enable `notebook_env`

Comment on lines +131 to +133
Copy link
Contributor

@gmanchon gmanchon Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than introducing a second use case for the package, why not systematically use dill so that lambdas are supported and the content writers do not need to wonder what they will be saving (which in time might be a source of bugs) in order to deal with that additional notebook_env parameter ?

if we keep a single use case, then the code only needs to deal with legacy content :

save :

  • save as dill file
  • remove pickle file if exists (no risk when the students run the code)

load :

  • load from dill file
  • fallback to load from pickle file

... then we can amend the package in the future when no content uses pickle anymore

WDYT ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dill can load any file created with pickle so I don't think we need the fallback. Are you suggesting we abstract away the saving of the notebook env?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I miss read the dump_session function 🙌

it is unclear to me why we need to save the state of the student __main__ module : should not the content of the tested functions be independent from the state ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do this

custom_ones = lambda x: np.ones(x)

you could not then pass this function alone

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see the issue (I expect the code loading the dill files to deal with the package imports)

Screenshot 2023-07-18 at 12 31 11

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is specific to how pytest operates

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I get the issue with the numpy import when using the ChallengeResult class. Even if I manually import numpy the np.ones in the lambda fails to find numpy

stackoverflow also points to dump_session

Copy link
Contributor

@gmanchon gmanchon Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ogiles1999 I found the ugliest hack to have pytest work with the numpy reference in the lambda 🙈

test_solvers.py :

from nbresult import ChallengeResultTestCase
import numpy as np

import __main__
__main__.np = np

class TestSolvers(ChallengeResultTestCase):
    def test_custom_ones(self):
        self.assertEqual([[1.0], [1.0], [1.0]], self.result.custom_ones([3, 1]).tolist())

Test Dill.ipynb :

import numpy as np

custom_ones = lambda x: np.ones(x)

custom_ones([3, 1])
from nbresult import ChallengeResult

result = ChallengeResult(
    'solvers',
    custom_ones=custom_ones
)
result.write()
print(result.check())

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha that is very hacky 😓

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One part of me is not proud 😁

```python
function_to_test = lambda x: x * 2

from nbresult import ChallengeResult

result = ChallengeResult("double", notebook_env=True, double_function=function_to_test)
```

This allows multiple test cases of the function to be tested in `double_test.py` by accessing `self.result.double_function`

## Testing

Run `make`
15 changes: 11 additions & 4 deletions nbresult/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""
nbresult package is for Le Wagon bootcamps students
"""
import pickle
import dill
import os
import unittest
import re
Expand All @@ -17,9 +17,10 @@ class ChallengeResult:
by `make` to validate challenge outcome)
"""

def __init__(self, name, subdir=None, **kwargs):
def __init__(self, name, subdir=None, notebook_env=False, **kwargs):
self.name = name
self.subdir = subdir
self.notebook_env = notebook_env
for key, value in kwargs.items():
setattr(self, key, value)

Expand Down Expand Up @@ -49,7 +50,10 @@ def write(self):
{self.name}, one is way too big.""")
result_file = os.path.join(tests_path, f"{self.name}.pickle")
with open(result_file, 'wb') as file:
pickle.dump(self, file)
dill.dump(self, file)
if self.notebook_env:
result_env = os.path.join(tests_path, f"{self.name}_env.pickle")
dill.dump_session(result_env)

def check(self):
"""returns test output on the ChallengeResult"""
Expand Down Expand Up @@ -87,7 +91,10 @@ def setUp(self):
name = re.sub(r'(?<!^)(?=[A-Z])', '_', klass).lower()[len('test_'):]
result_file = _locate_pickle(name)
with open(result_file, 'rb') as file:
self.result = pickle.load(file)
self.result = dill.load(file)
env_path = result_file.replace('.pickle', '_env.pickle')
if os.path.exists(env_path):
dill.load_module(env_path)


def _locate_pickle(name):
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@

dill
4 changes: 4 additions & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
with open("README.md", "r", encoding="utf-8") as fh:
long_description = fh.read()

with open('requirements.txt') as f:
required = [c.strip() for c in f.readlines()]

setup(name='nbresult',
version='0.0.9',
description='Extract results from Jupyter notebooks',
Expand All @@ -14,5 +17,6 @@
author='Kevin Robert',
author_email='[email protected]',
packages=find_packages(include=["nbresult"]),
install_requires=required,
include_package_data=True
)