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

Mkdocs gallery and sys.path #82

Merged
merged 10 commits into from
Nov 21, 2023
Merged

Mkdocs gallery and sys.path #82

merged 10 commits into from
Nov 21, 2023

Conversation

Louis-Pujol
Copy link
Contributor

Hello,

I have a little issue with sys.path and mkdocs-gallery. I realized that changes to sys.path were not persistent across code blocks. Here is a simple illustration script :

"""
PYTHONPATH
==========
"""

# %%
# Copy python path

import sys
original_path = sys.path.copy()


# %%
# Append path and print new path directories

sys.path.append("/home/mypath")
new_path = sys.path.copy()
for p in new_path:
    if p not in original_path:
        print(p)


# %%
# Print new path directories

path2 = sys.path.copy()
for p in path2:
    if p not in original_path:
        print(p)

And the resulting example:
Capture d’écran du 2023-11-14 15-13-53

sys.path Were reinitialized between block 2 and block 3 as /home/mypath is no longer inside sys.path.This behavior comes from this function, which is called between blocks and reset sys.path:

https://github.com/smarie/mkdocs-gallery/blob/d4c2d7fd7991a590abc2c947a61334b3a56ab9f7/src/mkdocs_gallery/gen_single.py#L731-#L735

This caused me some trouble because I use some libraries that add cache directories to sys.path. If I understand correctly the purpose of _reset_cwd_path, it is meant to remove cwd from sys.path, but it actually removes all new additions.

In this PR, I propose to modify _reset_cwd_path in order to remove only the last cwd and do not erase other modifications to sys.path. With this new version, my example works as I expected:

Capture d’écran du 2023-11-14 15-25-39

Let me know what are your thoughts about this changes.
Many thanks,
Louis

@smarie
Copy link
Owner

smarie commented Nov 20, 2023

Thanks @Louis-Pujol for this proposal !
I made a few readability comments (although the original sphinx-gallery code is not the best in readability, but we can at least marginally add comments)

Could you please also add a changelog entry for your mod ? Thanks !

Louis-Pujol and others added 3 commits November 20, 2023 10:30
Co-authored-by: Sylvain Marié <sylvain.marie@schneider-electric.com>
Co-authored-by: Sylvain Marié <sylvain.marie@schneider-electric.com>
@Louis-Pujol
Copy link
Contributor Author

Thanks, I committed your suggestions.

I'm not sure about what I need to do for adding a changelog entry. Do I need to create a new release on my repository ? And if yes, which tag do I need to choose ? Sorry if my questions are naive, I'm not very familiar with those things

@smarie
Copy link
Owner

smarie commented Nov 20, 2023

Thanks @Louis-Pujol . No, simply modify the file docs/changelog.md : add a bullet point in the first section (0.7.9 - (In Progress)) stating your contribution. You can follow the example that is already there

@Louis-Pujol
Copy link
Contributor Author

Thanks, I just did it.

@smarie
Copy link
Owner

smarie commented Nov 21, 2023

Thanks @Louis-Pujol . Now I had some afterthought: we do not want any sys path leak to happen between one example file and the others.

So now that sys path is preserved across blocks of a single example, we have to reset the sys path when the example is completed. Could you please modify parse_and_execute (that is calling all the execute_code_block) so that original sys path is copied before the loop, and it is reset after the loop ? That should do the trick.

Thanks a lot !

@@ -3,6 +3,7 @@
### 0.7.9 - (In Progress)

- Swapped from deprecated `disutils.version` to `packaging.version`. PR [#79](https://github.com/smarie/mkdocs-gallery/pull/79) by [Samreay](https://github.com/Samreay)
- Rewrited `gen_single._reset_cwd_syspath` function to let `sys.path` modifications persist accross blocks. PR [#82](https://github.com/smarie/mkdocs-gallery/pull/82) by [Louis-Pujol](https://github.com/Louis-Pujol).
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
- Rewrited `gen_single._reset_cwd_syspath` function to let `sys.path` modifications persist accross blocks. PR [#82](https://github.com/smarie/mkdocs-gallery/pull/82) by [Louis-Pujol](https://github.com/Louis-Pujol).
- `sys.path` modifications now persist accross blocks of an example. `sys.path` is still reset after each example. PR [#82](https://github.com/smarie/mkdocs-gallery/pull/82) by [Louis-Pujol](https://github.com/Louis-Pujol).

@Louis-Pujol
Copy link
Contributor Author

Thank you for pointing this out. I did the resetting of sys.path with a copy.deepcopy() + merge the last changes.

@smarie
Copy link
Owner

smarie commented Nov 21, 2023

Perfect @Louis-Pujol ! Thanks a lot. Will this this now. Two releases on the same day :D

@smarie smarie merged commit 0ad323b into smarie:main Nov 21, 2023
13 checks passed
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.

None yet

2 participants