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

Upgrade dependencies version support #95

Merged
merged 51 commits into from
Oct 19, 2021
Merged

Upgrade dependencies version support #95

merged 51 commits into from
Oct 19, 2021

Conversation

gmaze
Copy link
Member

@gmaze gmaze commented Sep 10, 2021

Will close #94

@gmaze gmaze added internals Internal machinery invalid This doesn't seem right labels Sep 10, 2021
Fix warnings in utilities tests
Use more appropriate variable name in filter_data_mode
Fix error raised in data_mode filtering. Now merge successively arrays by pair instead of 3 at a time
Refactoring of search functions in the  wmo filter
improve docstrings
make uri index store a property, not a method
@gmaze
Copy link
Member Author

gmaze commented Sep 17, 2021

as pointed by @quai20 in #63 (comment) a major change in fsspec occured after 0.8.3 that is a source of error for argopy caching functionalities:

Check out the output of the following code for different fsspec versions:

loader = ArgoDataFetcher(src='localftp', cache=True).float(2901623)
loader.load()

import pickle
fn = os.path.join(loader.fetcher.fs.fs.storage[-1], "cache")
cache = loader.fetcher.fs.fs.cached_files[-1]
if os.path.exists(fn):
    with open(fn, "rb") as f:
        cached_files = pickle.load(f)
else:
    cached_files = cache
print('cached_files keys:')    
[print("\t", k) for k in cached_files.keys()];

fsspec 0.8.3

This is the version currently supported by argopy:

cached_files keys:
	 file:///Users/gmaze/data/ARGO/ftp_current/dac/nmdis/2901623/2901623_prof.nc

fsspec 0.8.4 > fsspec 2021.08.1

Up to the last fsspec release, output is:

cached_files keys:
	 /Users/gmaze/data/ARGO/ftp_current/dac/nmdis/2901623/2901623_prof.nc

For some reasons fsspec does not prepend the path with the store protocol anymore, and this leads to CacheFileNotFound errors all over the place !
I could trace this change back to version fsspec 0.8.4

Fix

This is due to this peace of code being removed after 0.8.3 in fsspec/implementations/cached.py

if not path.startswith(self.target_protocol):
            store_path = self.target_protocol + "://" + path
else:
            store_path = path

This will be fixed by adding a test on fsspec version in our filesystems store_path method

Clean logging details
Fix monitor status to use the local ftp validator instead of the option checker
Commit lastest changes
Skip everything requiring cache/http for wmo erddap requests
changed domain and assume fsspec version compatibilitity
Fix bug avoiding argovis tests
Refactor/rename some tests
@gmaze
Copy link
Member Author

gmaze commented Sep 28, 2021

Despite improved understanding of what's going on (#96 ) using the erddap index fetcher is unstable ! :-(

Using my browser to fetch this region works, but it fails with the index fetcher.
Same thing with this float.

I just can't reproduce the error happening on the Github action tests ...

with long api timeout
Increase api timeout to 3 mins
Remove cache from all index/erddap
https://docs.python.org/3/library/exceptions.html#NotImplementedError:
"In user defined base classes, abstract methods should raise this exception when they require derived classes to override the method"
Fix bug in merged conflict
fix bug in abstract class test
fix bug in abstract method
Better list of dependancies in ci tests
@gmaze gmaze changed the title Upgrade dependancies version support Upgrade dependencies version support Oct 5, 2021
@gmaze
Copy link
Member Author

gmaze commented Oct 6, 2021

to @quai20 for your review, this PR content:

Addressed incompatibility with last versions of dependencies

Other changes

  • fix all flake8/lint errors (hence a bunch of noqa) and run black on some of the files
  • enforce uri to be a property not a class for all fetchers
  • follow recommendations on some of the class abstracts (eg: raise an NotImplementedError instead of a pass in abstract methods)
  • add a logger, mostly used in filesytems.py at this point.
  • clean up of the code here and there (eg: fix the issue in data fetcher plot where a full index was required, forgotten from Improve plotting modules #56 )
  • improved CI tests, add pytest.ini for logging, add tests for fetcher proto
  • improved (erddap_ds_exists, isconnected, show_versions, clear_cache) and added new utilities.py functions (show_options)
  • refactor xarray.py sub-method new_arrays into merge_arrays in the filter_data_mode method

@gmaze gmaze requested a review from quai20 October 6, 2021 08:29
scikit-learn<=1.0, >=0.21
netCDF4<=1.5.7, >=1.3.1
dask<=2021.09.1, >=2.9
toolz<=0.11.1, >=0.8.2
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 don't know why we have toolz as a requirement, we should try to remove it

Copy link
Member

Choose a reason for hiding this comment

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

I don't know about toolz yet, but packaging seems to be missing from the requirements on my side.

Copy link
Member

Choose a reason for hiding this comment

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

indeed, toolz does not seems to be necessary.

@gmaze gmaze self-assigned this Oct 8, 2021
@gmaze gmaze merged commit c0bb0ac into master Oct 19, 2021
@gmaze gmaze deleted the dependancies-upgrade branch October 19, 2021 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internals Internal machinery invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

argopy fails CI tests with latest dependancies
2 participants