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

Test onedata objectstore with new caching #4

Closed
wants to merge 39 commits into from

Conversation

bwalkowi
Copy link
Owner

@bwalkowi bwalkowi commented May 27, 2024

This PR complements this one: galaxyproject#18174

We have tested the deduplicated Onedada object store code and confirmed that it works.

We also took the occasion to include some improvements to our libs. They make the Onedata clients resistant to failures of data providers and improve their performance.

NOTE: the OnedataFileRestClient logs on the debug level, at least one log per request. Let us know if it's okay or maybe we should adjust the Galaxy's logger config so that these logs are ignored.

NOTE: while working on this, we have stumbled upon major performance problems which can also cause ambiguous data access errors, as described in galaxyproject#18369. This is why we are proposing to change the implementation of _get_total_matches_count to use scandir instead of glob in this PR.

[x] This is a refactoring of components with existing test coverage.

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@lopiola
Copy link
Collaborator

lopiola commented Jun 3, 2024

Proponuję docelowy opis PRa:

This PR complements this one: galaxyproject#18174

We have tested the deduplicated Onedada object store code and confirmed that it works.

We also took the occasion to include some improvements to our libs. They make the Onedata clients resistant to failures of data providers and improve their performance.

NOTE: the OnedataFileRestClient logs on the debug level, at least one log per request. Let us know if it's okay or maybe we should adjust the Galaxy's logger config so that these logs are ignored.

[x] This is a refactoring of components with existing test coverage.

@lopiola
Copy link
Collaborator

lopiola commented Jun 3, 2024

Najładniej by było chyba zrobić PRa do brancha jmchiltona, żeby jak sobie to wciągnie mogło wejść wszystko na raz z jego deduplikacją?

"onedatafilerestclient.http_client": {
"level": "INFO",
"qualname": "onedatafilerestclient.http_client",
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

zgaduję, że nie będą tego potrzebować więc póki co bym usunął

w opisie PRa zaproponowałem notkę dla nich do wyjaśnienia

@@ -223,9 +223,9 @@ auth:
# an access token suitable for data access (allowing calls to the Oneprovider REST API).
access_token: ...
connection:
# the domain of the Onezone service (e.g. "demo.onedata.org"), or its IP address for
# the domain of the Onezone service (e.g. datahub.egi.eu), or its IP address for
Copy link
Collaborator

Choose a reason for hiding this comment

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

thx

onedatafilerestclient==21.2.5rc1 ; python_version >= "3.8" and python_version < "3.13"
# onedatafilerestclient==21.2.5.1 ; python_version >= "3.8" and python_version < "3.13"
../onedatafilerestclient ; python_version >= "3.8" and python_version < "3.13"
../onedatarestfs ; python_version >= "3.8" and python_version < "3.13"
Copy link
Collaborator

Choose a reason for hiding this comment

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

to na koniec do ogarnięcia

@@ -7,8 +7,9 @@
from datetime import datetime

try:
from onedatafilerestclient import (
OnedataFileRESTClient,
from onedatafilerestclient import OnedataFileRESTClient
Copy link
Collaborator

Choose a reason for hiding this comment

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

wygląda na to że nie pykło z datahubem - dyskusja na Slaku

@bwalkowi bwalkowi force-pushed the test-onedata-objectstore-with-new-caching branch from ca05787 to 8802987 Compare June 6, 2024 08:45
mvdbeek and others added 26 commits June 7, 2024 15:20
This in particular needs a lot of new tests.
We will also need to actively purge datasets in the model store import
code, since users might have purged datasets while the job ran.
Again, more tests needed.
These are only written if the command actually ran, so we'd fail here
for instance if the outputs are deleted before the job had a chance to
run.
Add a sleep so we can delay output purging until command line is
templated. If we don't sleep we generate a command line where the
output path is just `''`. That needs another fix!
This is a tricky one. There could be systematic issues here that an
admin would want to fix, but it could also just be the case that a user
forced a wrong datatype. Ideally we'd probably tag this as job execution
issue ? It's not very different from a job error IMO.
For anonymous users, if a history_id is provided, do not override with the current session history and instead rely on the history accessibility
…ion_anonymous_jobs

[24.0] Fix anonymous user job retrieval logic
@bwalkowi bwalkowi force-pushed the test-onedata-objectstore-with-new-caching branch from 8802987 to 0b02038 Compare June 11, 2024 11:32
@bwalkowi bwalkowi force-pushed the test-onedata-objectstore-with-new-caching branch from 0b02038 to 2ec50fb Compare June 12, 2024 06:26
@bwalkowi bwalkowi closed this Jun 14, 2024
mvdbeek added a commit that referenced this pull request Sep 4, 2024
Fixes galaxyproject#18633:
```
ValueError: dictionary update sequence element #4 has length 1; 2 is required
  File "galaxy/tools/__init__.py", line 1969, in handle_single_execution
    rval = self.execute(
  File "galaxy/tools/__init__.py", line 2066, in execute
    return self.tool_action.execute(
  File "galaxy/tools/actions/model_operations.py", line 89, in execute
    self._produce_outputs(
  File "galaxy/tools/actions/model_operations.py", line 120, in _produce_outputs
    tool.produce_outputs(
  File "galaxy/tools/__init__.py", line 3816, in produce_outputs
    new_labels_dict = dict(source_new_label)

Exception caught while attempting to execute tool with id '__RELABEL_FROM_FILE__':
```
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.

8 participants