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

Adding functionality to Detect Running Environments for TARDIS Simulations #1650

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

DhruvSondhi
Copy link
Contributor

@DhruvSondhi DhruvSondhi commented Jun 14, 2021

This PR adds the functionality to check the environment in which the TARDIS Simulation is run.
Needs to be merged before the #1632

Description

This PR aims to add a new function check_simulation_env() which checks if the simulation is being run in a IPython environment or any other environment such as a Terminal, etc.
This function returns True in the former option & False in the latter.

Motivation and context

This function is vital for the implementation of the Logging Formatting which is being done in #1632. It would allow us to control the output of the logging & would allow for better formatting for the logging output generated while running the simulation.

How has this been tested?

  • Testing pipeline.
  • Other. Local Testing

Examples

Check PR #1632

Type of change

  • Bug fix.
  • New feature.
  • Breaking change.
  • None of the above.

Checklist

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
    • (optional) I have built the documentation on my fork following the instructions.
  • I have assigned and requested two reviewers for this pull request.

@tardis-bot
Copy link
Contributor

Before a pull request is accepted, it must meet the following criteria:

  • Is the necessary information provided?
  • Is this a duplicate PR?
    • If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
    • If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
  • Does it pass existing tests and are new tests provided if required?
    • The test coverage should not decrease, and for new features should be close to 100%.
  • Is the code tidy?
    • No unnecessary print lines or code comments.

@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #1650 (706b812) into master (66b10ad) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 706b812 differs from pull request most recent head ebecc9d. Consider uploading reports for the commit ebecc9d to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1650   +/-   ##
=======================================
  Coverage   61.92%   61.92%           
=======================================
  Files          62       62           
  Lines        5702     5713   +11     
=======================================
+ Hits         3531     3538    +7     
- Misses       2171     2175    +4     
Impacted Files Coverage Δ
tardis/tardis/util/base.py 76.92% <0.00%> (-0.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66b10ad...ebecc9d. Read the comment docs.

tardis/util/base.py Outdated Show resolved Hide resolved
return True
elif shell == "TerminalInteractiveShell":
return False
else:
Copy link
Member

Choose a reason for hiding this comment

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

when does this else statement happen?

Copy link
Contributor Author

@DhruvSondhi DhruvSondhi Jun 15, 2021

Choose a reason for hiding this comment

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

The following needs to be considered for the conditional check:

  • If we are in IPython Kernel environment, ie Jupyter Notebook, Jupyter Lab, etc, then we are in the ZMQInteractiveShell. Please check this.
  • If we are in the Terminal IPython environment, ie IPython Kernel running in Terminal, more of like a REPL, then we are in the TerminalInteractiveShell. Please check this.
  • If we are in any other environment, ie Google Colab or Normal Shell based Terminal, etc, then we are in the else condition. Hence, the main else condition allows us to print the values as a str in Formatting Logging Output for Simulation #1632

This Link explains exactly what I mean by these conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
This is Jupyter Notebook

image
This is IPython in terminal

image
*This is normal Python from command line

@DhruvSondhi
Copy link
Contributor Author

I think the Black tests are failing because of the comments present in the code 🤔

@DhruvSondhi DhruvSondhi force-pushed the detecting_run_env branch 2 times, most recently from a02c4d7 to bad49fb Compare June 15, 2021 16:15
@andrewfullard andrewfullard merged commit fa2925d into tardis-sn:master Jun 15, 2021
@DhruvSondhi DhruvSondhi deleted the detecting_run_env branch June 15, 2021 16:46
@Rodot-
Copy link
Contributor

Rodot- commented Jun 15, 2021

I would recommend instead of getting the class name from the dunder methods (not really part of their API), you should compare the shell directly using isinstance built-in function and comparing to ZMQInteractiveShell and InteractiveShell directly, which can be found through:

from IPython.kernel.zmq.zmqshell import ZMQInteractiveShell
and

from IPython.core.interactiveshell import InteractiveShell

@Rodot-
Copy link
Contributor

Rodot- commented Jun 15, 2021

Additionally, I do feel like the name check_simulation_environment is a little ambiguous given that it returns a bool rather than the simulation environment. Consider renaming it to something like is_ZMQInteractiveShell since that is the only condition for which it returns True. Or something like is_notebook if the intent is to check if the environment is a notebook (though, I don't know if ZMQInteractiveShell is always the interactive shell for notebooks)

Have you tested this in something like jupyter-lab or vscode?

@DhruvSondhi
Copy link
Contributor Author

Yes, Jupyter based Environments have the class name is always ZMQInteractiveShell. It is purely based on the IPython kernel being run or not. As for the screenshots above, you can see the different types of output that are returned when we run the get_ipython().__class__.__name__.

@DhruvSondhi
Copy link
Contributor Author

Additionally, I do feel like the name check_simulation_environment is a little ambiguous given that it returns a bool rather than the simulation environment. Consider renaming it to something like is_ZMQInteractiveShell since that is the only condition for which it returns True. Or something like is_notebook if the intent is to check if the environment is a notebook (though, I don't know if ZMQInteractiveShell is always the interactive shell for notebooks

Absolutely, I would change it in the logging formatting PR and hence we can make it little more clearer 😄

@DhruvSondhi
Copy link
Contributor Author

DhruvSondhi commented Jun 16, 2021

from IPython.kernel.zmq.zmqshell import ZMQInteractiveShell

Just a small update on this import. This has been deprecated in IPython 4 version. Newer import is the following:
from ipykernel.zmqshell import ZMQInteractiveShell

atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants