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

remove the conda/mamba based updater. #2495

Merged
merged 14 commits into from
Sep 6, 2024

Conversation

ReimarBauer
Copy link
Member

@ReimarBauer ReimarBauer commented Aug 30, 2024

Show in About a new version available.

Purpose of PR?:

Partial fix of #2256

removes the updater, because it is not sufficient when we release a pinning or when we install by pixi or a different package manager

Does this PR introduce a breaking change?
removes a feature

Copy link
Collaborator

@matrss matrss left a comment

Choose a reason for hiding this comment

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

Great to see this! Just some quick comments at first glance. Also, it doesn't actually address the issue described in #2256, the explicit reference to CONDA_PREFIX is still there.

mslib/utils/release_info.py Outdated Show resolved Hide resolved
mslib/utils/release_info.py Outdated Show resolved Hide resolved
mslib/mscolab/app/__init__.py Outdated Show resolved Hide resolved
ReimarBauer and others added 2 commits September 1, 2024 07:34
Co-authored-by: Matthias Riße <9308656+matrss@users.noreply.github.com>
different states for the release check
@ReimarBauer
Copy link
Member Author

Great to see this! Just some quick comments at first glance. Also, it doesn't actually address the issue described in #2256, the explicit reference to CONDA_PREFIX is still there.

I know, it is a first step, this part needs further changes. There needs more based on the mscolab_settings.

@ReimarBauer
Copy link
Member Author

There will be a seperate PR applying changes to the gallery workflow. This needs currently an update of the docker image.
I prefer to use the way how RTFD did it.

docs/installation.rst Outdated Show resolved Hide resolved
mslib/utils/qt.py Outdated Show resolved Hide resolved
mslib/utils/qt.py Outdated Show resolved Hide resolved
no_new_release_found = f"{datetime.date.today()}: No new release found."
try:
# we use the @concurrent.process(timeout=1) on the function to stop requests trying to establish a connection
latest_release = get_latest_release().result()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Immediately calling .result() IMO defeats the purpose of using pebble at all. What does this do that isn't already accomplished by just the timeout of requests.get?

Copy link
Member Author

Choose a reason for hiding this comment

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

requests.get timeout is used when the connection is established.

But it retrys 10 seconds until it gives up when there is no connection possible.

This is far to long for an user clicking on about, and on the server the startup or restart can be blocked too.

Copy link
Collaborator

@matrss matrss Sep 2, 2024

Choose a reason for hiding this comment

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

Do you have an example to reproduce the issue you see with it? The requests.get timeout specifies both the time to wait for a connection to be established and the time to wait for responses from the server. With timeout=(1, 1) it should also fail after at most 1 second if there is no connection possible (well, unless the target domain has multiple IP addresses, but that doesn't seem to be the case here).

The only other situation that I am aware of where the request might take longer is when the server is responding very slowly, waiting for almost the timeout before sending another few bytes. That seems to be an unlikely edge case though, especially when accessing GitHub's API?

Copy link
Member Author

@ReimarBauer ReimarBauer Sep 2, 2024

Choose a reason for hiding this comment

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

Just load the requests module and disconnect (or have no) internet before you execute

requests.get('https://duckduckgo.com/', timeout=(1,1))

with a connection available this shows a 200, without you wait until you get a requests.exceptions.ConnectionError

We can use MSS offline, we should not add something which can block this.

This behaves a bit different, it is faster than the about dialog

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 look on the gifcycle how it can block my system by commenting of the decorator and starting the local server
problem

Copy link
Member Author

Choose a reason for hiding this comment

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

Which network error do you have? and which requests version, I've just seen I hadn't the most recent.

Well we change our stack for our needs. When we can replace the other worker usages with it after the GSOC merges it would be usefull too.

I look if that concurrent.futures also helps.

Copy link
Member Author

@ReimarBauer ReimarBauer Sep 2, 2024

Choose a reason for hiding this comment

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

With requests 2.32.3 pyhd8ed1ab_0 conda-forge

seems we need to uplift the pinning

it seems to work

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/reimarb/Miniforge/envs/mssdev/lib/python3.11/site-packages/requests/api.py", line 73, in get
    return request("get", url, params=params, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/reimarb/Miniforge/envs/mssdev/lib/python3.11/site-packages/requests/api.py", line 59, in request
    return session.request(method=method, url=url, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/reimarb/Miniforge/envs/mssdev/lib/python3.11/site-packages/requests/sessions.py", line 589, in request
    resp = self.send(prep, **send_kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/reimarb/Miniforge/envs/mssdev/lib/python3.11/site-packages/requests/sessions.py", line 703, in send
    r = adapter.send(request, **kwargs)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/reimarb/Miniforge/envs/mssdev/lib/python3.11/site-packages/requests/adapters.py", line 700, in send
    raise ConnectionError(e, request=request)
requests.exceptions.ConnectionError: HTTPSConnectionPool(host='api.github.com', port=443): Max retries exceeded with url: /repos/Open-MSS/MSS/releases/latest (Caused by NameResolutionError("<urllib3.connection.HTTPSConnection object at 0x7862fb9f88d0>: Failed to resolve 'api.github.com' ([Errno -3] Temporary failure in name resolution)"))

Copy link
Member Author

Choose a reason for hiding this comment

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

mamba list -r

2024-09-02 18:09:17  (rev 6)
     certifi  {2024.7.4 (conda-forge/noarch) -> 2024.8.30 (conda-forge/noarch)}
     requests  {2.31.0 (conda-forge/noarch) -> 2.32.3 (conda-forge/noarch)}

2024-09-02 18:16:53  (rev 7)
     requests  {2.32.3 (conda-forge/noarch) -> 2.32.1 (conda-forge/noarch)}

2024-09-02 18:18:25  (rev 8)
     requests  {2.32.1 (conda-forge/noarch) -> 2.31.0 (conda-forge/noarch)}

Maybe the problem was only on my system? It is also gone currently with the old version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Which network error do you have?

As mentioned above:

requests.exceptions.ConnectionError: HTTPSConnectionPool(host='duckduckgo.com', port=443): Max retries exceeded with url: / (Caused by NameResolutionError("<urllib3.connection.HTTPSConnection object at 0x77467b735540>: Failed to resolve 'duckduckgo.com' ([Errno -2] Name or service not known)"))

So I am getting a "Errno -2 Name or service not known" while you are getting an "Errno -3 Temporary failure in name resolution".

Copy link
Member Author

Choose a reason for hiding this comment

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

I still can't reproduce the problem I had before. But I always now have Errno -3 independent from cable or wireless disconnection

@matrss
Copy link
Collaborator

matrss commented Sep 2, 2024

Great to see this! Just some quick comments at first glance. Also, it doesn't actually address the issue described in #2256, the explicit reference to CONDA_PREFIX is still there.

I know, it is a first step, this part needs further changes. There needs more based on the mscolab_settings.

That's fine, but then the PR description can't mention that it fixes that issue, when it really doesn't.

@ReimarBauer
Copy link
Member Author

It doesn't fix the name but in the discussion in the issue the problem of what needs to be changed has more facettes.

@matrss
Copy link
Collaborator

matrss commented Sep 2, 2024

It doesn't fix the name but in the discussion in the issue the problem of what needs to be changed has more facettes.

Yes, but merging this PR would auto-close that issue, while the issue isn't solved. And that is just plain wrong. If you want an issue attached to this PR, please make removing the updater a separate issue, since that was only brought up in #2256, but not the main point of that issue, or make removing the usage of CONDA_PREFIX part of this PR.

@ReimarBauer
Copy link
Member Author

It doesn't fix the name but in the discussion in the issue the problem of what needs to be changed has more facettes.

Yes, but merging this PR would auto-close that issue, while the issue isn't solved. And that is just plain wrong. If you want an issue attached to this PR, please make removing the updater a separate issue, since that was only brought up in #2256, but not the main point of that issue, or make removing the usage of CONDA_PREFIX part of this PR.

It is only a matter of the merge order. When it is already closed, it is connected which is also valueable.

from the issue "Some code assumes that MSS is installed with conda or mamba. This happens e.g. here:"

for me e.g. means lookup also others.

@matrss
Copy link
Collaborator

matrss commented Sep 2, 2024

OK, makes sense, then both this PR and #2499 must be merged at the same time, as only the combination of both addresses #2256.

@ReimarBauer
Copy link
Member Author

OK, makes sense, then both this PR and #2499 must be merged at the same time, as only the combination of both addresses #2256.

It is also common in the "unperfect" world of github to add comments to already closed issues. I splitted the PR from an undo or reading perspective.

Copy link
Collaborator

@matrss matrss left a comment

Choose a reason for hiding this comment

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

I've updated the PR description to reflect that this is a partial fix of said issue. LGTM now.

@ReimarBauer ReimarBauer merged commit ee9e832 into Open-MSS:develop Sep 6, 2024
10 of 11 checks passed
@ReimarBauer ReimarBauer deleted the remove_updater_i2256 branch September 6, 2024 06:17
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.

3 participants