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

test_eval: Show example of working closure #2743

Merged
merged 3 commits into from
Aug 6, 2021

Conversation

EricCousineau-TRI
Copy link
Collaborator

Description

Shows an example unittest with using py::exec with a closure.
I had forgotten what was necessary, then came across #1742 again. I'm adding an example here just to show it.

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

Ah, cool! I knew things were annoying and complicated, but not this annoying and complicated. Good to have a test on this to remind ourselves!


def func():
return closure_value
)", local, local);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is local being passed twice here? And from reading #1742, shouldn't this be called global as it's the globals dict?

@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Dec 25, 2020

Suggestion to expand this test, to show differences between what's captured and what's not captured with locals vs. globals:

    // test_eval_closure
    m.def("test_eval_closure", []() {
        py::dict global;
        global["closure_value"] = 42;
        py::dict local;
        local["closure_value"] = 0;
        py::exec(R"(
            local_value = closure_value

            def func_global():
                return closure_value

            def func_local():
                return local_value
            )", global, local);
        return std::make_pair(global, local);
    });

and

def test_eval_closure():
    global_, local = m.test_eval_closure()

    assert global_["closure_value"] == 42
    assert local["closure_value"] == 0

    assert "local_value" not in global_
    assert local["local_value"] == 0

    assert "func_global" not in global_
    assert local["func_global"]() == 42

    assert "func_local" not in global_
    with pytest.raises(NameError):
        local["func_local"]()

Maybe I'm misunderstanding/butchering the original aim of this test & PR, though? Please do tell me, then, if this extension is irrelevant or obfuscating to the closure construction you're trying to test, @EricCousineau-TRI!

@@ -96,4 +97,15 @@ TEST_SUBMODULE(eval_, m) {
auto int_class = py::eval("isinstance(42, int)", global);
return global;
});

m.def("test_eval_closure", [global, copy]() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe also add the // test_eval_closure comment above. In this case, it's a bit over the top since your function is already called that, but I like the existing convention of indicating the Python-side tests that C++-side bindings belong to.

@YannickJadoul
Copy link
Collaborator

expand this test

If you like the idea, I have this on my git's stash, btw, so I can just commit and push, if that saves trouble.

@EricCousineau-TRI
Copy link
Collaborator Author

If you like the idea, I have this on my git's stash, btw, so I can just commit and push, if that saves trouble.

Ooh, yeah, that'd be great! Though understandable if it's gotten lost in your stash after a month 😅
Just lemme know!

@YannickJadoul
Copy link
Collaborator

Luckily I hardly ever clear out that thing, so it was still somewhere down ;-)
(Also lucky you don't need to pop the stash like a true stack :-P )

@Skylion007 Skylion007 added docs Docs or GitHub info needs-review labels Aug 6, 2021
@Skylion007 Skylion007 added this to the v2.8 milestone Aug 6, 2021
@Skylion007 Skylion007 requested review from henryiii and rwgk August 6, 2021 19:29
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Just documenting that I looked, too :-)

@Skylion007 Skylion007 merged commit 6ac8efe into pybind:master Aug 6, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 6, 2021
@henryiii henryiii removed needs changelog Possibly needs a changelog entry needs-review labels Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Docs or GitHub info
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants