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

updated figure 1, inputted JRM written edits #102

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

jeremymanning
Copy link
Member

No description provided.

Copy link
Member

@paxtonfitzpatrick paxtonfitzpatrick left a comment

Choose a reason for hiding this comment

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

@jeremymanning I left some comments throughout on various changes! Happy to discuss anything

@@ -296,7 +300,7 @@ \subsubsection{The \texttt{smuggle} statement}\label{subsec:smuggle}
Functionally, importing Davos in an IPython notebook enables
an additional Python keyword: ``\texttt{smuggle}'' (see
Sec.~\ref{subsec:implementation} for details on how this works).
The \texttt{smuggle} keyword can be used as a drop-in
The \texttt{smuggle} keyword-like object can be used as a drop-in
Copy link
Member

Choose a reason for hiding this comment

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

I think we should refer to smuggle as a "keyword" here. Even though it's not a true a Python keyword, I think it's okay (and better) to do so here for a couple reasons:

  1. this section is about "software functionality"; we're trying to describe how davos is used. From a user's perspective, they use "smuggle" as a keyword.
  2. we preface the prior sentence with "Functionally" and point the reader to the relevant section "for details on how this works". IMO that's plenty to quickly convey to the reader that there's something going on behind the scenes and that smuggle may not be a "true" keyword in the traditional sense, while not introducing additional points of confusion that distract & detract from the overall point of this sentence: that smuggle is a drop-in replacement for the import keyword. i.e., I know what a keyword in Python is and how to use one; I've never heard of a "keyword-like object" before

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, that's fine

@@ -309,7 +313,7 @@ \subsubsection{The \texttt{smuggle} statement}\label{subsec:smuggle}
Importantly, packages installed by Davos are made available for use in the
notebook without affecting the user's Python environment or existing packages.
By default, \texttt{smuggle} statements will install missing packages (and any
missing dependencies of those packages) into a notebook-specific, virtual
missing dependencies of those packages) into a notebook-specific virtual
Copy link
Member

Choose a reason for hiding this comment

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

"notebook-specific" and "virtual environment-like" are coordinate adjectives describing "directory" and should have a comma between them

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@@ -391,26 +395,26 @@ \subsubsection{The onion comment}\label{subsec:onion}
\subsubsection{Projects}\label{subsec:projects}

Standard approaches to installing packages from within a notebook can alter the local Python environment in potentially unexpected and undesired ways.
For example, running a notebook that installs its dependencies via system shell commands (prefixed with ``\texttt{!}'') or IPython magic commands (prefixed with ``\texttt{\%}'') may cause other existing packages in the user's environment to be uninstalled and replaced with alternate versions.
For example, running a notebook that installs its dependencies via system shell commands (prefixed with ``\texttt{!}'') or IPython magic commands (prefixed with ``\texttt{\%}'') may cause other existing packages in the user's environment to be uninstalled or replaced with alternate versions.
Copy link
Member

Choose a reason for hiding this comment

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

installing a package would never cause a different package in the environment to be uninstalled without being replaced by a different version -- I was trying to capture how installing a package can result in other packages that are dependencies of the package being installed getting upgraded or downgraded (i.e., uninstalled and replaced with alternate versions) in order to satisfy the new package's requirements. This is what installing the new package in a Davos project prevents.

If you don't like the wording of "uninstalled and replaced", I think simply "replaced" would convey the same meaning just as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

let's go with "replaced"

This can lead to incompatibilities between installed packages, affect the behavior of the user's other scripts or notebooks, or even interfere with system applications.

To prevent Davos-enhanced notebooks from having unwanted side effects on the user's environment, any packages installed via \texttt{smuggle} statements are automatically isolated using a custom, virtual environment-like system called ``projects.''
To prevent Davos-enhanced notebooks from having unwanted side effects on the user's environment, any packages installed via \texttt{smuggle} statements are automatically isolated using custom virtual environment-like systems called ``projects.''
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is introducing the overall "system" or "scheme" that Davos uses to isolate smuggled packages -- I'm not sure a single project directory constitutes a "system"?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok...just reads strangely to me. fine with whatever though!

When \texttt{smuggle} statements within that notebook are then executed, any packages (or specific versions of packages) that are not already available in the user's Python environment are installed into the notebook's project directory (along with any missing dependencies of those packages).
During each \texttt{smuggle} statement's execution, Davos also temporarily prepends the notebook's project directory to the module search path so that these project-installed packages are visible when searching for smuggled packages locally, and prioritized over those in the user's main environment.
When \texttt{smuggle} statements within that notebook are executed, any packages (or specific versions of packages) that are not already available in the user's Python environment are installed into the notebook's project directory (along with any missing dependencies of those packages).
During each \texttt{smuggle} statement's execution, Davos also temporarily prepends the notebook's project directory to the module search path so that these project-installed packages are visible when searching for smuggled packages locally, and prioritized over those in the runtime environment.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think "runtime environment" is quite the right term to describe the user's existing Python environment as distinct from the packages Davos makes available via the project directory.

IME the definition of "runtime environment" for Python is a bit fuzzier than for other languages with a clearer distinction between compile time and run time, but in general I think of "Python runtime environment" as referring to "everything available to the Python interpreter at runtime". So "project-installed packages" are part of the runtime environment because Davos makes them available to the interpreter by prepending the project directory to sys.path. The use of "runtime environment" in the next sentence after this one (line 410) makes sense.

One example from an official source: https://www.python.org/success-stories/securing-python-runtimes

a runtime environment is defined as the Python language itself + popular third party packages + the interpreter.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok...i don't feel strongly about this one

@@ -520,7 +524,7 @@ \subsubsection{Configuring and querying Davos}\label{subsec:config}

\end{itemize}

\noindent The current values of all \texttt{davos} attributes may be viewed at once within a notebook by displaying the \texttt{davos.config} object.
\noindent The current values of all \texttt{davos} attributes may be viewed at once within a notebook by printing the \texttt{davos.config} object.
Copy link
Member

@paxtonfitzpatrick paxtonfitzpatrick Sep 29, 2023

Choose a reason for hiding this comment

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

"displaying" $\neq$ "printing" in IPython/a Jupyter notebook.
I was trying to convey that you can just type davos.config in a notebook cell, run it, and view the current config values. When you do that in IPython or a Jupyter notebook, the output you're shown is not equivalent to print(foo), it's equivalent to

from IPython.display import display
display(foo)

A pandas.DataFrame is a good example of this.

I'd like to keep this distinction in there (i.e., revert "printing" to "displaying") because even though the output of displaying and printing davos.config is currently the same, it likely won't be in the future. The difference between printing & displaying in a notebook comes from the fact that printing an object shows the output of its __str__() method (which, if not defined, falls back to its __repr__() method), while displaying shows the output of the object's _repr_html_() method if one is defined, and falls back to its __repr__() if not. Since the purpose of davos.config is now pretty much entirely for inspecting config options rather than setting them, I'm planning to add a _repr_html() method to make this look nicer

Copy link
Member Author

Choose a reason for hiding this comment

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

got it. that's fine then

@@ -703,7 +707,7 @@ \section{Illustrative Example}\label{sec:illustrative-example}
\end{center}
It is worth noting, however, that beyond illustrative purposes, the benefit of specifying only a maximum version for \texttt{joblib} rather than an exact version is relatively minor.
The main advantage to relaxing a version constraint in an onion comment (when a package's behavior does not differ meaningfully between versions) is that doing so increases the likelihood that a satisfactory version will already be available in the user's Python environment, and therefore Davos will not need to install a new copy in the notebook's project directory.
For large packages, this can be a worthwhile consideration; however \texttt{joblib} is very lightweight---less than 0.5 MB pre-built, with no required dependencies.
For large packages, this can be a worthwhile consideration; however \texttt{joblib} is very lightweight---less than 0.5 MB pre-built, with no other dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

"required dependencies" was an intentional phrasing here because se draw on the fact that joblib has an optional dependency on numpy 2 paragraphs below this. This is also how joblib itself makes the distinction: https://pypi.org/project/joblib/

Copy link
Member Author

Choose a reason for hiding this comment

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

got it. ok, feel free to revert

Comment on lines +1015 to +1029
While Davos enables developers to conveniently specify all project
dependencies, there are some edge cases and limitations that are worth
considering. Many Python packages include (in their setup options) additional
dependencies that often carry their own version specifications. Although Davos
will check that the correct version of the requested top-level package is
installed and imported into the workspace, the version numbers of any
\textit{dependencies} of the requested package are not checked. In principle,
this could lead to unexpected behavior, for example if a given package's
dependencies (or dependencies of those dependencies, etc.) were left
under-specified. A developer could mitigate this by explicitly smuggling exact
version numbers of every project dependency (e.g., obtained via pip freeze).
However, for projects where the versions of dependencies of smuggled packages
also need to be precisely controlled, a lockfile or a \texttt{requirements.txt}
file produced by \texttt{pip freeze} (i.e., explicitly specifying \textit{all} packages'
version numbers) may provide a more comprehensive alternative to Davos.
Copy link
Member

Choose a reason for hiding this comment

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

I think adding this back in might've been unintentional? We had talked about this, agreed it doesn't make sense, and you had removed it a few months back (and we confirmed that earlier today via Slack as well)

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for the reminder. fine to cut this

Copy link
Member

@paxtonfitzpatrick paxtonfitzpatrick left a comment

Choose a reason for hiding this comment

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

thanks @jeremymanning! I'll merge these and implement my comments accordingly

@paxtonfitzpatrick paxtonfitzpatrick merged commit 74193be into ContextLab:main Sep 29, 2023
1 check passed
paxtonfitzpatrick added a commit to paxtonfitzpatrick/davos that referenced this pull request Sep 30, 2023
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.

2 participants