Skip to content

Conversation

LouiseABowler
Copy link

Will resolve #82.

I've switched the initial introduction of the line-profiler package over to the new method recommended to run it. I've tested out on MacOS and all is fine so far; next week I'll have access to Ubuntu and Windows machines so I'll check that section on each of those operating systems before going any further with the set of changes.

Copy link

github-actions bot commented May 9, 2025

Thank you!

Thank you for your pull request 😃

🤖 This automated message can help you check the rendered files in your submission for clarity. If you have any questions, please feel free to open an issue in {sandpaper}.

If you have files that automatically render output (e.g. R Markdown), then you should check for the following:

  • 🎯 correct output
  • 🖼️ correct figures
  • ❓ new warnings
  • ‼️ new errors

Rendered Changes

🔍 Inspect the changes: https://github.com/RSE-Sheffield/pando-python/compare/md-outputs..md-outputs-PR-85

The following changes were observed in the rendered markdown documents:

 md5sum.txt         |  4 +--
 profiling-lines.md | 75 +++++++++++++++++++++++++++++++++++++++++++++++-------
 setup.md           | 12 +++++++--
 3 files changed, 78 insertions(+), 13 deletions(-)
What does this mean?

If you have source files that require output and figures to be generated (e.g. R Markdown), then it is important to make sure the generated figures and output are reproducible.

This output provides a way for you to inspect the output in a diff-friendly manner so that it's easy to see the changes that occur due to new software versions or randomisation.

⏱️ Updated at 2025-05-10 19:36:59 +0000

github-actions bot pushed a commit that referenced this pull request May 10, 2025

If you are unable to install `line_profiler` via `pip` on MacOS. Instead it can be installed via `conda`.
If you come across the error message `zsh: no matches found: line_profiler[all]`, try wrapping the package name in quotation marks:
Copy link
Member

Choose a reason for hiding this comment

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

Nice find! 😮
I'm very much not a mac user, and work on Windows 90% of the time (the 10% being where I use Linux purely to test/resolve Linux build issues).

@Robadob
Copy link
Member

Robadob commented May 10, 2025

next week I'll have access to Ubuntu and Windows machines so I'll check that section on each of those operating systems before going any further with the set of changes.

Fairly sure it should work on Linux, I've used similar in the past.

Windows cmd and powershell however are not so happy (does work under git bash and other nix style terminals)

C:\Users\Robadob\pando\pando-python\episodes\files>LINE_PROFILE=1 python lp.py
'LINE_PROFILE' is not recognized as an internal or external command,
operable program or batch file.
(base) PS C:\Users\Robadob\pando\pando-python\episodes\files> LINE_PROFILE=1 python lp.py
LINE_PROFILE=1 : The term 'LINE_PROFILE=1' is not recognized as the name of a cmdlet, function,
script file, or operable program. Check the spelling of the name, or if a path was included,
verify that the path is correct and try again.
At line:1 char:1
+ LINE_PROFILE=1 python lp.py
+ ~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (LINE_PROFILE=1:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CommandNotFoundException

Windows equivalent seems to be

cmd /c "set LINE_PROFILE=1 && python lp.py"

E.g. setting the env var within a temporary child instance of cmd.

That's kind of grim, probably cleaner to suggest Windows users just toggle it on with a separate set LINE_PROFILE=1 command.

Probably a good place to use Grouped tabs to differentiate Windows vs everything else

@JostMigenda
Copy link
Collaborator

Should we just follow the example of core carpentries lessons and recommend Windows users use Git bash or WSL, where compatibility is better?

CMD and Powershell are different enough that it’ll be a non-trivial amount of work to check and adjust all instructions; plus, learners using completely different shells makes it harder for instructors to help if any problems pop up during the course.

@Robadob
Copy link
Member

Robadob commented May 11, 2025

Should we just follow the example of core carpentries lessons and recommend Windows users use Git bash or WSL, where compatibility is better?

CMD and Powershell are different enough that it’ll be a non-trivial amount of work to check and adjust all instructions; plus, learners using completely different shells makes it harder for instructors to help if any problems pop up during the course.

I'd err on the side of having a Windows exception in a tab/group just for simplicity sake. I don't think any of the other commands should really be affected as they're just running scripts which is common across everything.

Atleast in Sheffield, the vast majority of attendees are Windows users, so making things more difficult for them seems like a bad idea (high chance some are not git users and won't have WSL2 setup on their laptops).

@Robadob
Copy link
Member

Robadob commented May 11, 2025

I haven't tested it, but maybe setting os.environ["LINE_PROFILE"] = "1" right at the top of the Python file, before the import of line profiler would work and be common? (arguably a little grim though)

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.

Update method used to run line-profiler
3 participants