Skip to content

Conversation

@huandzh
Copy link

@huandzh huandzh commented Jul 23, 2019

@CLAassistant
Copy link

CLAassistant commented Jul 23, 2019

CLA assistant check
All committers have signed the CLA.

@huandzh
Copy link
Author

huandzh commented Jul 23, 2019

odps/tests/test_config.py tested and passed with pandas 0.25.0.

Copy link
Contributor

@qinxuye qinxuye left a comment

Choose a reason for hiding this comment

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

Great job, but I think we can use try except other than version comparison.

if self._use_pd:
import pandas as pd
from pandas.core.config import OptionError as PandasOptionError
if pd.__version__ < '0.25.0':
Copy link
Contributor

Choose a reason for hiding this comment

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

We could try import pandas.core.config, if failed, try to import pandas._config.config. Documents can be added about pandas version.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we can do that now.

But it seems not right to handle compatibility issues with exceptions (exceptions are most likely to be something go unexpected).

OptionError is private in '0.25.0' , but may be published and move to another place afterwards. We can't handle updates directly with ImportError then.

Any idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think try except is a better way. How about just make a modification?

We cannot predict the behavior of pandas in the future, let's fix the current issue first.

Copy link
Author

Choose a reason for hiding this comment

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

Could you given some more reasoning? I really can't understand why try except is preferred in this use case. Importing from pandas.core.config will sure fail in current and future versions.

And code already committed here did fix the issue.

See also:

pandas-dev/pandas#27553
pandas-dev/pandas#27656

Copy link
Contributor

@qinxuye qinxuye left a comment

Choose a reason for hiding this comment

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

Could you please resolve the problem by try except way? thanks.

if self._use_pd:
import pandas as pd
from pandas.core.config import OptionError as PandasOptionError
if pd.__version__ < '0.25.0':
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think try except is a better way. How about just make a modification?

We cannot predict the behavior of pandas in the future, let's fix the current issue first.

@qinxuye
Copy link
Contributor

qinxuye commented Aug 15, 2019

Sorry to tell you that we have solved the compatible issue in a way that pandas version does not matter. Refer to

def getvalue(self, silent=False):
if self._use_pd:
import pandas as pd
try:
return pd.get_option('.'.join(self._items))
except (KeyError, LookupError, AttributeError):
self._use_pd = False
else:
return self._val

Thank you all the same for your contribution, I will close this PR anyway, welcome to send other PRs.

@qinxuye qinxuye closed this Aug 15, 2019
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