Python Multi Version Test Suite #946
Conversation
|
should we add Python 3.10 there too? its twice as popular as 3.5, if you go by global pypi download numbers since the beginning of this year |
|
Yep, let's add 3.10 and 3.11. @valayDave we would have to install Rcpp from source for R3.5 and R3.6. It's not worth the hassle at the moment, so we can safely disable those tests for now. |
metaflow/datastore/task_datastore.py
Outdated
| value = v.decode("utf-8") | ||
| else: | ||
| value = v | ||
| ret_dict[k] = json.loads(value) if v is not None else None |
There was a problem hiding this comment.
Is this correct? if we expect v to be None, then L:495 will break.
There was a problem hiding this comment.
I agree that this needs to change. we should have a
if v is None:
ret_dict[k] = None
continue
at the top and then we can do this part.
I would actually suggest something like this:
transformer = lambda x: x
if sys.version_info < (3, 6):
transformer = lambda x: x.decode("utf-8")
return {
k : json.loads(transformer(v)) if v is not None else None for k, v in self._load_file(names, add_attempt).items()
}
It's a tad more efficient and a bit easier to read maybe.
metaflow/datastore/flow_datastore.py
Outdated
| @@ -1,5 +1,6 @@ | |||
| import itertools | |||
| import json | |||
| import sys | |||
| @@ -158,7 +159,7 @@ def get_latest_task_datastores( | |||
| elif fname == TaskDataStore.METADATA_DATA_SUFFIX: | |||
| # This somewhat breaks the abstraction since we are using | |||
| # load_bytes directly instead of load_metadata | |||
| with open(path, "rb") as f: | |||
| with open(path, encoding="utf-8") as f: | |||
There was a problem hiding this comment.
I am not familiar with this part of the code. @romain-intel is this the right thing to do here?
There was a problem hiding this comment.
This looks ok yes. The data is stored with save_metadata which does a json.dumps followed by an encode("utf-8") so this should be reasonable to open as text with that encoding.
| @@ -158,7 +159,7 @@ def get_latest_task_datastores( | |||
| elif fname == TaskDataStore.METADATA_DATA_SUFFIX: | |||
| # This somewhat breaks the abstraction since we are using | |||
| # load_bytes directly instead of load_metadata | |||
| with open(path, "rb") as f: | |||
| with open(path, encoding="utf-8") as f: | |||
There was a problem hiding this comment.
This looks ok yes. The data is stored with save_metadata which does a json.dumps followed by an encode("utf-8") so this should be reasonable to open as text with that encoding.
metaflow/datastore/task_datastore.py
Outdated
| value = v.decode("utf-8") | ||
| else: | ||
| value = v | ||
| ret_dict[k] = json.loads(value) if v is not None else None |
There was a problem hiding this comment.
I agree that this needs to change. we should have a
if v is None:
ret_dict[k] = None
continue
at the top and then we can do this part.
I would actually suggest something like this:
transformer = lambda x: x
if sys.version_info < (3, 6):
transformer = lambda x: x.decode("utf-8")
return {
k : json.loads(transformer(v)) if v is not None else None for k, v in self._load_file(names, add_attempt).items()
}
It's a tad more efficient and a bit easier to read maybe.
|
Addressed all comments + nit fixes. Tests are passing. Seems ready to merge. |
Changes
publish.ymlare refactored to come fromtest.yml. We reuse the jobs in the Test gh action. Workflows being reused need to have on.workflow_call. This helps reuse it locally.