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

Add -c option: program passed as string #61

Merged
merged 2 commits into from
May 6, 2022
Merged

Add -c option: program passed as string #61

merged 2 commits into from
May 6, 2022

Conversation

tal66
Copy link
Contributor

@tal66 tal66 commented Apr 25, 2022

Hi, this closes #44.

can now run:

  • memray run -c 'print("my python commands")' -> a py file will be created with this string as its content. then arg.script will be replaced with this file's name. after that, memray runs as before.
  • memray run -c <my_file>.py -> parser.error with help message to remove -c option

(EDIT: no longer creating a file)

@tal66
Copy link
Contributor Author

tal66 commented Apr 25, 2022

Hi, some tests failed because I changed output to the console. didn't expect it to be tested :)
e.g

assert error:
E         - Only Python files can be executed under memray
E         + Only Python files (or valid Python commands using -c) can be executed under memray

I can fix the tests, but see if in general this looks fine to you

@pablogsal
Copy link
Member

Hi @tal66 and thanks for your contribution!

Two things: can you add a test for this in the test suite. Please, check the format of other CLI-related tests.

Also, per our contribution licensing documentation, would you mind adding a Signed-Off-By: tag to your commit message to indicate that you have read and agree to our Developer's Certificate of Origin?

That should be as easy as doing:

$ git commit --amend -C HEAD -s
$ git push -f

from a checkout of this branch. But for now, don't worry about it (even less if you used the GitHub UI :) )

@godlygeek
Copy link
Contributor

godlygeek commented Apr 26, 2022

I'd prefer a different approach here. Instead of writing the command to a file, I'd prefer to add another case to _run_tracker here:

if args.run_as_module:
runpy.run_module(args.script, run_name="__main__", alter_sys=True)
else:
runpy.run_path(args.script, run_name="__main__")

That's where we currently handle either running a file or running a module. I'd add another case for running a command, and do:

            if args.cmd:
                exec(args.cmd, {"__name__": "__main__"})
            elif args.run_as_module:
                runpy.run_module(args.script, run_name="__main__", alter_sys=True)
            else:
                runpy.run_path(args.script, run_name="__main__")

@tal66
Copy link
Contributor Author

tal66 commented Apr 27, 2022

Hi, can you approve running the workflows on the last push? (used --force)

@pablogsal
Copy link
Member

Hi, can you approve running the workflows on the last push? (used --force)

Done!

Use as follows: `memray run -c '<write python commands here, or paste py file contents. then close the quote>'`

Signed-off-by: tal66 <[email protected]>
@tal66
Copy link
Contributor Author

tal66 commented Apr 27, 2022

I just changed the signoff now, that was the only problem in the checks

Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to review this. This looks very good to me, though I have a few thoughts about it.

I'm happy to make these changes myself on top of what has already been committed here, but I'm curious to hear other people's thoughts on them.

src/memray/commands/run.py Show resolved Hide resolved
src/memray/commands/run.py Outdated Show resolved Hide resolved
src/memray/commands/run.py Outdated Show resolved Hide resolved
Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

Looks good to me - thank you for the contribution, @tal66!

@godlygeek godlygeek merged commit b21908a into bloomberg:main May 6, 2022
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.

Add run -c mode to allow program passed in as string
3 participants