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

Conversation

olivergiles
Copy link

Added the possibiltiy to use functions define in the notebook inside the tests. You add an additional notebook_env=False and then you can pass functions into the tests!

@olivergiles olivergiles requested a review from gmanchon July 17, 2023 14:31
Copy link
Contributor

@gmanchon gmanchon left a comment

Choose a reason for hiding this comment

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

good idea to use dill, I suggest to systematically use dill and only fallback to pickle, WDYT ?

setup.py Outdated Show resolved Hide resolved
Comment on lines +131 to +133
### 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`

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 😁

Co-authored-by: Gaëtan Manchon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants