Enable uninstalling requests

[rickwierenga]

I was surprised to learn BayBE includes telemetry so deep in the project. It's uncomfortable importing a package that talks to the internet.

Since

"BayBE collects anonymous usage statistics only for employees of Merck KGaA, Darmstadt, Germany and/or its affiliates. The recording of metrics is turned off for all other users and impossible due to a VPN block."

these dependencies seem useless for most users?

In this PR, I moved requests to the # Attempt telemetry import section so it's no longer needed to run anything in this project.

Because of #181, the telemetry is no longer optional to install when using pyproject.toml? I just installed the dependencies manually. It'd be nice to have this be optional so users don't get surprised by an automatic requests installed. Does poetry not do that?

---

Also, is there a reason this is not mentioned in the README? It used to be, but was removed: #3. Why?

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ Scienfitz
❌ rickwierenga
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Scienfitz]

Hi @rickwierenga

the problem is that we have no way of making dependencies opt-out. In the ideal world we would allow installation via something like pip install baybe[notelemetry]. This would then install the code with telemetry deactivated and not require any internet packages. But to the best of my knowledge thats not possible.

As a result we decided to have internet packages in the dependencies. The reason for that is simple: no one would opt-in for telemetry, thats is normal for any product and software - even if the only target group are our company-internal internal users I think no one would opt in. Telemetry packages were never optional to install, there was an optional dependency group which was however referenced in the main deps (#181 merely removed that because poetry cant deal with that)

For externals, we had hoped that

• the fact that this is an open-source package where people can see exactly what data is potentially uploaded (we'd only upload anonymized stats that don't include any direct data or measurements, we don't even log the parameter names etc)
• the fact that telemetry is auto-deactivated if the company-internal reference page is not reachable (aka you're not in our VPN means telemetry does not send anything)
• the fact that you can also deactivate telemetry in addition via environment variables as explained here

would suffice.

But I can see that if you want to be really sure you'd also want to not have any internet package in your environment. If that is what your PR enables then I'd be willing to accept it. I guess it would still require you to do some manual uninstalling after baybe installation or how are you dealing with it?

I think the telemetry mention was moved from the README because we wanted to reduce content there. But if it helps we can reintroduce the mention or at least a link to the detailed explanation.

[rickwierenga]

Thanks for the quick and detailed answer. I see the points about this being open source (which is awesome!) but I think unfortunately not everyone will take the time to read it.

Why not make it opt-in?

[Scienfitz]

@rickwierenga well, just briefly thinking about the amount of newsletters / telemetry measurements I have opted-in when I was asked for it amounts to pretty much zero. I don't think even >10% of our internal scientists would opt-in.

Until we find a better way to enable opt-out I would probably favor the solution that users like you have to go for the manual deinstall of packages they don't want (keeping in mind that less suspicious users still have 3 ways of deactivating or verifying that nothin sus is going on). We can mention that also in the telemetry explanations.

[rickwierenga]

Sounds good! I'll try this out for my experiments this night and tomorrow, but if results are good I'm happy to work on this!

[AdrianSosic]

Hi @rickwierenga 👋 thanks for connecting. I think @Scienfitz explained our dilemma quite accurately. To provide a little more context: baybe was originally developed purely for inhouse optimization, but we pushed to make the library open-source and were in fact successful with this plan. However, has now brought us into a sort of chimera state where the telemetry part is still "inhouse" in a sense. And I think you will understand that we can't simply drop it because the gathered internal usage stats eventually justify our work on the library – without them, there would have been no package in the first place.

That said, we are hoping to find a better solution in the mid term, ideally via opt-out. In fact, there is an ongoing discussion on extending pip capabilities in that direction, but it hasn't yet progressed very far. Let's see what the happens in the next few months.

Until then, I think your PR is a good first step 👍🏼

[Scienfitz]

@rickwierenga were you able to test whether this works for you, ie uninstalling the internet deps + your code change? If you confirm I will add updates to the telemetry descriptions and merge this

[AdrianSosic]

Sounds good! I'll try this out for my experiments this night and tomorrow, but if results are good I'm happy to work on this!

Hi @rickwierenga, the other PR #205 that adds a telemetry section to the README is up. Ideally, we'd like to wait for your final confirmation before merging both. Can you give a quick update if everything works as expected for you?

[AdrianSosic]

Hi @Scienfitz, seems like @rickwierenga is currently busy with other things. Could you include that additional paragraph to the readme so that we can proceed towards merging?

[Scienfitz]

@AdrianSosic will do so but I will check whether the uninstall actually enables what was claimed before I merge

[AdrianSosic]

@AdrianSosic will do so but I will check whether the uninstall actually enables what was claimed before I merge

Jep, makes sense. Let me know if I should do something...

[rickwierenga]

nice! I like the new transparency :)

[Scienfitz]

tested, proceeding to merge now

[Scienfitz]

Hi @rickwierenga
This PR is accepted and ready for merge (I rebased your fork and added README and CHANGELOG entries fyi)

But if you want this PR merged with your association you still need to sign the CLA agreement, as pointed out by the bot (second comment in this PR)

[AdrianSosic]

Hi @rickwierenga, just briefly wanted to inform you that we plan to merge this PR before releasing 0.9.0, which will happen at some point during this week. We'd love to have your association with this PR, so I encourage you to sign the CLA the latest until tomorrow. Of course, you don't need to sign if you refuse, but then we'll probably have to merge these changes from a different branch. So hoping you will accept 🙃

[Scienfitz]

@rickwierenga We recently removed the requests package alltogether in #240
As a result, this PR has become obsolete and Im closing it. Thanks nonetheless for raising the issue about potential trust issues, the README got telemetry mention again and internet packages (such as opentelemetry which I suspect still has requests as secondary dependency) can be uninstalled if desired