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

Detect Project.toml in parent dirs and disable PlutoPkg #1281

Closed
wants to merge 2 commits into from

Conversation

fonsp
Copy link
Owner

@fonsp fonsp commented Jul 1, 2021

this PR does not work yet, but join the discussion below!

One 'problem' with the new built-in package manager is:

What to do when your notebook's folder or one of its parents has a Project.toml?

For example, you could have a project structured like:

my_project/
    data/
        ...
    notebooks/
        Interesting analysis.jl
        sample Tower of Hanoi.jl
        ...
    Project.toml
    Manifest.toml
    ...

and you want all notebooks to use the same package environment.

Why not one environment per notebook?

Because two notebooks using the same package might produce slightly different results, if they are not created at the same time. This can be very confusing. This is also important when you import code from one notebook in the other.

What happens in main?

Currently, you need to write

import Pkg
Pkg.activate(joinpath("..",".."))

to disable PlutoPkg and enable your shared environment. This is also recommended in our docs: https://github.com/fonsp/Pluto.jl/wiki/%F0%9F%8E%81-Package-management#pattern-the-shared-environment

What happens in 0.14.x?

I only recently discovered that this behaviour is inconsistent (), but:

  • If you launched Pluto inside the folder, then that folder will be active for all notebooks, regardless of where they are stored.
  • If you launched Pluto outside the folder, then all notebooks will use the global environment, even if they are stored inside the folder.

Solution 1 - this PR

After this PR, when running the notebooks inside the notebooks/ folder, Pluto will detect that they are part of a shared project environment, and PlutoPkg will be disabled (no inline GUI). The project will be activated.

When moving a notebook file in/out of a shared folder, PlutoPkg will be disabled/re-enabled.

This will simulate the julia --project=@. behaviour for all notebooks:

search through parent directories until a Project.toml or JuliaProject.toml file is found.

This matches the behaviour of Jupyter and VS Code: JuliaLang/Pkg.jl#2640 (comment)

TODO

TODO

  • Detect
  • Disable
  • Use AbstractPackageManagement instead of Union{PkgContext, Nothing}
  • Set the parent dir as ACTIVE_PROJECT[] or sth
  • Show to the user
  • Update docs

TO TEST

  • Notebook in shared dir activates the dir
  • Notebook in shared dir with Pkg.activate does not activate the dir
  • Notebook moved out of shared dir becomes pluto-managed
  • Notebook moved into shared dir becomes pluto-managed
  • notebook_to_js contents
  • reset_notebook_environment

Bonus ideas:

  • Include project data in the .jl file generated using the export button
  • Detect change in shared env and recommend restart

@fonsp fonsp added the package manager Pluto's built-in package manager label Jul 1, 2021
@lungben
Copy link
Contributor

lungben commented Jul 2, 2021

I sometimes have example notebooks in a subdir of a package where I would like to use the new Pluto Pkg feature. These example notebooks usually have different dependencies than the package they are in, therefore the package Project.toml would not be suitable to use.
It would be good to have some kind of opt-in or opt-out for this behavior.

I think opt-in with automatic migration could do it:
If there is a call to PlutoUseParentProject() (or whatever syntax) then the automatic Pkg management is deactivated and the behavior described in this PR is used. Otherwise, behavior of current main is ued (new Pkg management or manual calls to Pkg).
For any notebook which is opened with a Pluto version tag of < 0.15 PlutoUseParentProject() is added automatically (with a suitable command, saying that it should be removed to get automatic Pkg management) to be compatible with existing notebooks.

This would have the advantage to be explicit about the behavior of taking parent Project.tomls (which otherwise would be quite intransparent) and it pushes towards using the new Pkg functionality (which is in most cases the better choice).

@fonsp
Copy link
Owner Author

fonsp commented Jul 2, 2021

What you mentioned is a (valid) edge case, but this PR will cover a more common use case, right? I think it's fine to not enable the pluto package manager for that edge case, you can still use Pkg.activate(temp=true) to set up an environment yourself.

The goal for PlutoPkg is to make common use cases work automatically, without any extra steps/code, and more specialized use cases require extra code from the user. So I still think this PR is the best way to go.

One point you mentioned, that this behaviour is intransparent, is a good one. I think that we should solve this by showing it in the GUI. Something like the current PlutoPkg bubble, except it always says "this notebook is using the environment at ... you can edit it using the Pkg REPL"

@aplavin
Copy link
Contributor

aplavin commented Jul 3, 2021

I made that comment regarding VS Code and IJulia behaviour that you refer to, but it was not intended as a suggestion for Pluto to follow the same direction! It’s great that you have a fresh take on the notebook environment, and choosing best defaults is a part of it.

Currently, Pluto 0.15 (master) uses the new package manager UI by default with a reasonably clear way of escaping it: the activate(…) function. It’s already easy to use a project from a parent directory with activate(current_project()) – doesn’t require any changes in Pluto.

On the other hand, if you switch from the current approach to this PR, there would be no way to use the Pluto package manager when there happens to be a Project.toml file in any higher-level directory.

So, I believe keeping the current (master, not this PR) defaults is best along the “reproducible by default” line, and generally for convenience of one-off notebooks.

@lungben
Copy link
Contributor

lungben commented Jul 3, 2021

I think this PR is required so that Release 0.15 does not break existing notebooks which rely on a Project.toml in the current or a parent directory. But I agree with you that this behavior is not ideal and long-term users should use the new Pluto package management in most cases.

I suggest to implement this PR but with a Deprecation Warning each time a Project.toml is used. The warning should give the syntax to explicitly activate the current (or parent) directory for using the Project.toml.
In a future Pluto release (e.g. 0.16) using directory Project.toml as default should be rolled back and the Pluto Pkg manager be used as default (regardless of the existence of a Project.toml).

@aplavin
Copy link
Contributor

aplavin commented Jul 3, 2021

I use Project.toml files myself, and don't see how Pluto 0.15 can break them - unless the Pkg.activate call is buried somewhere in included code, and not in the notebook itself. If there is no Pkg.activate call, then Pluto 0.14- just uses the global julia env, it definitely didn't activate any parent directory automatically.
Do I miss something here?

@Datseris
Copy link

Datseris commented Jul 3, 2021

After this PR, when running the notebooks inside the notebooks/ folder, Pluto will detect that they are part of a shared project environment, and PlutoPkg will be disabled (no inline GUI). The project will be activated.

I think this is a good idea. Not yet clear to me whether this would still work with nested folders inside notebooks, e.g. notebooks/firstpaper/test.jl. But I guess if you use the source of Base.current_project it will.

One point you mentioned, that this behaviour is intransparent, is a good one. I think that we should solve this by showing it in the GUI. Something like the current PlutoPkg bubble, except it always says "this notebook is using the environment at ... you can edit it using the Pkg REPL"

+1

Unfortunately I am a bit at a loss about what is the current behavior. I've looked here: https://github.com/fonsp/Pluto.jl/wiki/%F0%9F%8E%81-Package-management but from what I've read the behavior is to always use the internal Pluto manager. From here https://github.com/fonsp/Pluto.jl/wiki/%F0%9F%8E%81-Package-management#pattern-the-shared-environment if someone calls Pkg.activate, then this is disabled. This is the status quo at the moment, correct?


For notebooks that are part of a ongoing scientific project, where it is unlikely that your analysis would finish within a single notebook, I think it is much more beneficial that their shared environment is activated. Another benefit is when giving paths to data files. Sure, you can put the data file in the same folder as the notebook, but for a real project this will quickly become unfeasible. A sufficiently unique identifier of the entire project folder would be the location of the Project.toml, from which you can branch out to everything you need, including data files, source julia files, or notebooks. (which is how a large part of DrWatson works).

@fonsp
Copy link
Owner Author

fonsp commented Jul 5, 2021

#1281 (comment) @lungben @aplavin this PR is required so that Release 0.15 does not break existing notebooks which rely on a Project.toml in the current or a parent directory

This is a bit confusing, but because of an implementation bug (#1280), 0.14 did not activate the parent directory in this case. So we do not need this PR for backwards compatibility of this behaviour.

@fonsp
Copy link
Owner Author

fonsp commented Jul 5, 2021

#1281 (comment) @aplavin It’s already easy to use a project from a parent directory with activate(current_project()) – doesn’t require any changes in Pluto.

I agree, but Base.current_project() is internal API. (FYI to others:) I made an API request to Pkg to add Pkg.activate(recursive=true), @aplavin replied but I did not get much response from Pkg: JuliaLang/Pkg.jl#2640

@fonsp
Copy link
Owner Author

fonsp commented Jul 5, 2021

An alternative to this PR is:

I set the Base.ACTIVE_PROJECT[] to @. for notebooks that call Pkg.activate. This means that a notebook with:

import Pkg; Pkg.activate()

(no arguments) will activate the parent environment if it exist, the global env otherwise.

This means that you don't need to write the complicated Pkg.activate(joinpath("..", "..")) (there is no easier API yet: JuliaLang/Pkg.jl#2640)

@lungben
Copy link
Contributor

lungben commented Jul 5, 2021

I think it is perfectly fine to use a Project.toml in the notebook directory itself automatically, this is a behavior which is probably expected by most users. Using a Project.toml from a parent directory may be rather surprising, but fortunately it looks like it is not required to do this for backward compatibility reasons.

To take the Project.toml from a parent directory one could also write
Pkg.activate(Base.current_project())
Source: JuliaLang/Pkg.jl#2640 (comment)
This should be done explicitly in the notebook in my opinion.

@Datseris
Copy link

Datseris commented Jul 5, 2021

Using a Project.toml from a parent directory may be rather surprising

I find this surprising, and interesting. How does your project folder structure look like? Where do you put your data? Do you write your own source code for e.g. analysis functions, or you have everything in a massive notebook file? If you write source code files, where do you put them? Just curious, because coming from a different perspective, I'd find it more natural to group things into data/source/notebook folders.

@greimel
Copy link
Contributor

greimel commented Jul 5, 2021

Here is something that I would like from a teaching perspective. (And sorry if this functionality is already implied from one of your posts above)

I have a folder notebooks/ with many notebooks. I want my students to use my custom sysimage, so there is a common Project and Manifest file notebooks/*.toml.

But I also want my students to be covered if they open the notebook naïvely just dragging the path into Pluto.

So, it would be good if Pluto copied the tomls to the .jl file even if Pluto uses an external Pkg environment, so that it can also be used standalone.

You could for example have a pop-up with something like

Pluto detected a package enviroment in the notebook's folder. Pluto is about to replace the notebook's package environment with it. Shall it proceed?

@aplavin
Copy link
Contributor

aplavin commented Jul 5, 2021

By the way, I encourage everyone to try the Pluto package manager :)

Originally, I was somewhat skeptical whether it would be convenient and intuitive compared to project-based directories that are plain Julia envs with Project.toml. But after about a week, I really enjoy the understanding that each notebook is completely self-contained regarding julia packages. Move a notebook to another project? No issues! Install an incompatible or bleeding edge package for some specific part of analysis within a project? Fine!

I hope that even if this PR gets merged and Pluto pkg manager won't be the default anymore, it would still be possible to override: e.g., Pluto.which_management(...) = Pluto.FullyManaged, nothing before running Pluto.

@Datseris
Copy link

Datseris commented Jul 5, 2021

Perhaps a pop-up is the best way? If Pluto detects whether a notebook is part of a larger project, by finding a Project.toml in current, or parrent folders, a pop-up comes out that says "Found project give/directory/to/project/! Should we activate this project, or handle the notebook via Pluto's package manager?" Then the user just clicks a yes/no question, and doesn't have to fiddle with internals like Pluto.FullyManaged.

@j-fu
Copy link
Contributor

j-fu commented Jul 6, 2021

Not sure if a pop-up on start is the best idea here as it would prevent automatization.

So basically I tend to agree with the idea to use Pkg.activate(Base.current_project()).

If an interactive UI becomes necessary, we could add a "Project Management" UI element to what is now the print panel along with a project status indicator which is always visible close to the Move widget. This could/should be arbitrarily verbose to support inexperienced users.

@fonsp
Copy link
Owner Author

fonsp commented Jul 6, 2021

Thanks for all the input! There is something to say for both directions, so my final decision is:

  • Close this PR, leave the behaviour as is. This keeps the logic simple to understand.
  • Recommend Pkg.activate(Base.current_project()) in the documentation.

In the future, we could:

About Base.current_project()

While not public API, I think it is safe to assume that it will continue to be supported (especially if Pluto users start using it). In the meantime, we should make a PR to Julia to document the function. Does this sound good?

@fonsp fonsp closed this Jul 6, 2021
@Datseris
Copy link

Datseris commented Jul 6, 2021

Just be careful: Base.current_project() uses pwd() as directory. It should be Base.current_project(@__DIR__).

@fonsp
Copy link
Owner Author

fonsp commented Jul 6, 2021

Added an inline widget to communicate the Pkg.activate behaviour: #1293

@fonsp
Copy link
Owner Author

fonsp commented Jul 6, 2021

@Datseris We set the pwd to the notebook directory, and a cell with Pkg.activate is prioritized to run before any other code, that's why I left that detail away in the docs.

@Datseris
Copy link

Datseris commented Jul 6, 2021

Sure, but I thought a big selling point of Pluto is that its notebooks are also "true" Julia files, i.e. one could in principle run them on e.g. other means. Then not using @__DIR__ would fail. Or do I remember incorrectly and Pluto files are not also true runnable Julia files?

@dralletje dralletje deleted the detect-shared-project-env branch November 6, 2021 00:03
@dralletje dralletje restored the detect-shared-project-env branch November 6, 2021 00:03
@dralletje dralletje deleted the detect-shared-project-env branch November 6, 2021 00:03
@dralletje dralletje restored the detect-shared-project-env branch November 6, 2021 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package manager Pluto's built-in package manager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants