Skip to content

Add -c option: program passed as string#61

Merged
godlygeek merged 2 commits into
bloomberg:mainfrom
tal66:run--c-option
May 6, 2022
Merged

Add -c option: program passed as string#61
godlygeek merged 2 commits into
bloomberg:mainfrom
tal66:run--c-option

Conversation

@tal66
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Contributor Author

tal66 commented Apr 27, 2022

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

@pablogsal
Copy link
Copy Markdown
Collaborator

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 <77445020+tal66@users.noreply.github.com>
@tal66
Copy link
Copy Markdown
Contributor Author

tal66 commented Apr 27, 2022

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

Copy link
Copy Markdown
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.

Comment thread src/memray/commands/run.py
Comment thread src/memray/commands/run.py Outdated
Comment thread src/memray/commands/run.py Outdated
Co-authored-by: Matt Wozniski <godlygeek@gmail.com>
Copy link
Copy Markdown
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