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

Sir/python_3114 #211

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

sirEven
Copy link

@sirEven sirEven commented Sep 3, 2023

This PR solves compatibility issues when using dydx-v3-python with python 3.11.4
The issues stem from conflicting imports by the parsimonious package, which is used by eth-abi package, which in turn is used by both eth-account and web3 packages.

The following github issues seem related to this PR:

What this PR does:

@christophlins
Copy link

I would like to install the client for v3 alongside v4 for Python 3.11.x via poetry.

$ poetry add git+https://github.com/sirEven/dydx-v3-python.git#sir/python_3114
$ poetry add git+https://github.com/dydxprotocol/v4-clients.git@main#subdirectory=v4-client-py

Unfortunately, this raises a conflict with the sympy dependency.
How about changing the fixed version for sympy to an upper version constraint in preparation for v4 i.e. ^1.12.0?

@Otnicka1
Copy link

Hello guys,
wen this PR will be merged ?

@sirEven
Copy link
Author

sirEven commented Oct 24, 2023

I would like to install the client for v3 alongside v4 for Python 3.11.x via poetry.

$ poetry add git+https://github.com/sirEven/dydx-v3-python.git#sir/python_3114
$ poetry add git+https://github.com/dydxprotocol/v4-clients.git@main#subdirectory=v4-client-py

Unfortunately, this raises a conflict with the sympy dependency. How about changing the fixed version for sympy to an upper version constraint in preparation for v4 i.e. ^1.12.0?

Hey there, i just tried to up the constraint to sympy<=1.12.0 - which would be fine as far as i can tell!
How ever there is now one test failing (independent of your requested requirements change): Apparently somehow there seems to be a user called "foo" on the ganache test network all of a sudden, such that the test "def test_check_if_username_exists(self):" fails because it returns true instead of false - while i think this is hilarious, i don't quite understand how this has come to be... I posted a question in the dev channel on discord to resolve this issue. My quick fix currently is to check for "fooo" instead in this test to make it green again. However i decided to wait a little while on some feedback before proceeding once i can push, i let you know so you can use the new version with the desired constraints! Cheers, S.

Edit: Never mind my hesitation, i just upped the constraint to your needs and fixed the test along the way by changing the user name to smth else. I hope your undertaking works now!
S.

@christophlins
Copy link

Edit: Never mind my hesitation, i just upped the constraint to your needs and fixed the test along the way by changing the user name to smth else. I hope your undertaking works now! S.

Many thanks! I am just wondering why you wouldn't change it to sympy>=1.12.0. This reflects the dependency constraint in v4.

@sirEven
Copy link
Author

sirEven commented Oct 25, 2023

Edit: Never mind my hesitation, i just upped the constraint to your needs and fixed the test along the way by changing the user name to smth else. I hope your undertaking works now! S.

Many thanks! I am just wondering why you wouldn't change it to sympy>=1.12.0. This reflects the dependency constraint in v4.

Hey man, so sorry, i misunderstood you! I Hope it works now for you as i just pushed them to >= 1.12.0 :)
Cheers, S.

@MementoRC
Copy link

MementoRC commented Oct 25, 2023

@christophlins Do you have any plans to make a repo source release? I have prepared a conda recipe, but since there is not a repo release, I used the pypi package with heavy modifications (along the lines of @sirEven PR). This may not be accepted (recipe maintainers are not allowed to make changes)
Incidentally, is there someone interested in being a co-maintainer for the conda recipe?
Recipe (in PR state): conda-forge/staged-recipes#23806

@christophlins
Copy link

@christophlins Do you have any plans to make a repo source release?

No. For the time being, I rely on this PR until it gets merged and eventually releases by the dydx folks.

@christophlins
Copy link

@sirEven

Unfortunately, more dependencies mismatches popped up... Comparing v3 with v4 requirements, I think the following changes are necessary:

cytoolz>=0.12.1
dateparser>=1.0.0
ecdsa>=0.18.0
eth-account>=0.9.0
mpmath>=1.3.0
requests>=2.31.0
six>=1.16.0
web3>=6.5.0

Would you mind giving it a spin? If it doesn't work straight away, it might not be worth the effort...

@sirEven
Copy link
Author

sirEven commented Oct 27, 2023

@sirEven

Unfortunately, more dependencies mismatches popped up... Comparing v3 with v4 requirements, I think the following changes are necessary:

cytoolz>=0.12.1
dateparser>=1.0.0
ecdsa>=0.18.0
eth-account>=0.9.0
mpmath>=1.3.0
requests>=2.31.0
six>=1.16.0
web3>=6.5.0

Would you mind giving it a spin? If it doesn't work straight away, it might not be worth the effort...

hey @christophlins I just upped the dependencies as you suggested, let me know if that helps!
Cheers, S.

@christophlins
Copy link

hey @christophlins I just upped the dependencies as you suggested, let me know if that helps! Cheers, S.

@sirEven It works like a charm. Many thanks for the quick turnaround. Highly appreciated!

@MementoRC
Copy link

@christophlins Cool, I mistakenly thought you were a maintainer

No. For the time being, I rely on this PR until it gets merged and eventually releases by the dydx folks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants