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.

Sign up for GitHub

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

Jump to bottom

fix: add warning when using defaults #3434

Merged
merged 10 commits into from
Sep 12, 2024
Merged

fix: add warning when using defaults #3434

merged 10 commits into from
Sep 12, 2024

Conversation

wolfv
Copy link
Member

@wolfv wolfv commented Sep 7, 2024

Following all the drama about Anaconda's aggressive behavior towards users who are using the defaults channel (discussed at the NumFOCUS summit) we came up with the plan to at least add a warning.

I had a quick go at adding a warning when using defaults.

We should discuss what the UX should be like:

  • A warning that can be disabled either via ENV var or via .condarc setting
  • We could also go one step further and have a y/n prompt, after which we would append a value to the .condarc file.

Which option would you all prefer?

@wolfv
Copy link
Member Author

wolfv commented Sep 7, 2024

cf conda/conda#14217

@SylvainCorlay
Copy link
Member

I missed the conversation at the summit, so I cannot comment on that.

I think that it makes sense for us to help mamba users, and warning them that using certain channels may incur costs as per their TOS makes sense IMO.

However, conda is a client of this library and we should probably make sure that they can disable this warning if they decided to.

@jjerphan jjerphan mentioned this pull request Sep 9, 2024
Closed
@jjerphan jjerphan added the release::bug_fixes For PRs fixing bugs label Sep 9, 2024
Hind-M
Hind-M reviewed Sep 9, 2024
View reviewed changes
libmamba/src/api/channel_loader.cpp Outdated
@@ -63,6 +63,10 @@ namespace mamba
{
for (const auto& platform : channel.platforms())
{
if (channel.platform_url(platform).host() == "repo.anaconda.com") {
spdlog::warn("This is a commercial channel hosted by Anaconda.com. Please make sure you understand the TOS.");
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 can use LOG_WARNING instead and avoid including <iostream>.

@Hind-M
Copy link
Member

Hind-M commented Sep 9, 2024

We should discuss what the UX should be like:

* A warning that can be disabled either via ENV var or via `.condarc` setting

* We could also go one step further and have a `y/n` prompt, after which we would append a value to the `.condarc` file.

Which option would you all prefer?

I think having something via the .condarc would be enough.
The y/n prompt seems to be a good idea because that makes the user actually read the message, but I'm not sure how this could interfere with the tests and people's workflow, and how much it could be annoying...

@jjerphan
Copy link
Member

jjerphan commented Sep 10, 2024 •
edited
Loading

The y/n prompt is blocking if called in user scripts; so I guess the first option is the best.

@jjerphan jjerphan changed the title add warning when using defaults fix: add warning when using defaults Sep 10, 2024
JohanMabille
JohanMabille reviewed Sep 10, 2024
View reviewed changes
Copy link
Member

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

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

We still need a way to deactivate this warning. I would go for a full configurable option (rc, env and public API exposed to Python) so that clients can choose how they disable it.

@jjerphan
Copy link
Member

We still need a way to deactivate this warning. I would go for a full configurable option (rc, env and public API exposed to Python) so that clients can choose how they disable it.

How would you best do that?

JohanMabille
JohanMabille reviewed Sep 10, 2024
View reviewed changes
libmamba/src/api/channel_loader.cpp Outdated Show resolved Hide resolved
libmamba/src/api/configuration.cpp Outdated Show resolved Hide resolved
@JohanMabille
Copy link
Member

Let's remove the test for now, there are some issues with the capture and redirection of stdout and stderr that are orthogonal to this PR.

@jjerphan jjerphan mentioned this pull request Sep 11, 2024
Open
@jjerphan
Copy link
Member

See #3440.

wolfv and others added 9 commits September 12, 2024 01:02
@wolfv @SylvainCorlay
add warning when using defaults
de97b15
@jjerphan @SylvainCorlay
Format
5fe33ff 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@jjerphan @Hind-M @SylvainCorlay
Use LOG_WARNING
62c2ba3 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Co-authored-by: Hind Montassif <hind.montassif@gmail.com>
@jjerphan @SylvainCorlay
Adapt log message
4f826be 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@jjerphan @SylvainCorlay
Add show_anaconda_channel_warnings
bac48fd 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@jjerphan @SylvainCorlay
Expose show_anaconda_channel_warnings
a221a8d 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@jjerphan @JohanMabille @SylvainCorlay
Address review comments
f7f8286 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Co-authored-by: Johan Mabille <johan.mabille@gmail.com>
@jjerphan @SylvainCorlay
Remove tests about warnings
ba94972 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@SylvainCorlay
Disable output validation
227697c
@SylvainCorlay SylvainCorlay force-pushed the add-warning branch 3 times, most recently from ded04c4 to 2630bbd Compare September 12, 2024 14:55
@jjerphan jjerphan mentioned this pull request Sep 12, 2024
Closed
1 task
@SylvainCorlay SylvainCorlay force-pushed the add-warning branch 2 times, most recently from fa3ced0 to c847160 Compare September 12, 2024 16:28
@jjerphan jjerphan merged commit a261cd4 into mamba-org:main Sep 12, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hind-M Hind-M Hind-M left review comments

@SylvainCorlay SylvainCorlay SylvainCorlay approved these changes

@JohanMabille JohanMabille JohanMabille approved these changes

Assignees
No one assigned
Labels
release::bug_fixes For PRs fixing bugs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wolfv @SylvainCorlay @Hind-M @jjerphan @JohanMabille