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

Arweave Integration Milestone 1: Publish using transaction ID #860

Closed

Conversation

MantisClone
Copy link
Contributor

@MantisClone MantisClone commented Jun 17, 2022

Warning
Revert the following commit before (or after) merging this PR. This commit skips the code coverage step in the CI which fails because CC_TEST_REPORTER_ID is not set in PRs by external contributors.

Warning
Revert the following commit before (or after) merging this PR. This commit skips test_nonocean_tx which fails because REMOTE_TEST_PRIVATE_KEY1 == '' empty string in PRs by external contributors

Towards oceanprotocol/pm#151.

Changes proposed in this PR

  • Add ArweaveFile class and test_arweave_file() to test it.
  • Add get_arweave_file() helper and arweave_file fixture.
  • Make existing tests use the arweave_file fixture. including: test_encrypt, test_fileinfo, test_initialize, test_consume_flow, test_compute_flow_registered_asset_in_arweave, and all of the tests in test_ocean_assets.py
  • When downloading a file stored in Arweave, replace filename with random UUID hex so filename doesn't leak transaction ID.

Note
This does not include the ability to upload to Arweave. That is left for a future PR.

Changes that were implemented but then reverted

@MantisClone MantisClone changed the title Add ArweaveFile class Arweave Integration Jun 17, 2022
@MantisClone
Copy link
Contributor Author

MantisClone commented Jun 19, 2022

I believe all the changes related to Arweave files are working as intended.

All the tests are passing except the polygon config test due to error:

E requests.exceptions.HTTPError: 429 Client Error: Too Many Requests for url: https://rpc.polygon.oceanprotocol.com/

https://github.com/oceanprotocol/ocean.py/runs/6950317363?check_suite_focus=true#step:7:150

I believe this error is unrelated to the Arweave integration changes.

@MantisClone MantisClone marked this pull request as ready for review June 19, 2022 11:58
MantisClone and others added 18 commits June 20, 2022 11:00
ImportError while loading conftest '/home/runner/work/ocean.py/ocean.py/conftest.py'.
conftest.py:9: in <module>
    from ocean_lib.aquarius.aquarius import Aquarius
ocean_lib/aquarius/__init__.py:6: in <module>
    from .aquarius import Aquarius  # noqa
ocean_lib/aquarius/aquarius.py:17: in <module>
    from ocean_lib.assets.asset import Asset
ocean_lib/assets/asset.py:13: in <module>
    from ocean_lib.services.service import Service
ocean_lib/services/service.py:19: in <module>
    from ocean_lib.structures.file_objects import FilesType
ocean_lib/structures/file_objects.py:11: in <module>
    class FilesType:
ocean_lib/structures/file_objects.py:32: in FilesType
    def from_dict(cls, dict: Dict[str, Any]) -> Self:
/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/site-packages/enforce_typing/decorator.py:8: in enforce_types
    spec = inspect.getfullargspec(wrapped)
/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/inspect.py:1144: in getfullargspec
    raise TypeError('unsupported callable') from ex
E   TypeError: unsupported callable
@AnaLoznianu
Copy link
Member

AnaLoznianu commented Oct 11, 2022

Hi @MantisClone! I've added the reviewers back. I see the tests are failing though, can you please fix them?

@MantisClone MantisClone temporarily deployed to CC_REPORTER_ID October 11, 2022 13:14 Inactive
@MantisClone MantisClone temporarily deployed to CC_REPORTER_ID October 11, 2022 14:29 Inactive
@MantisClone
Copy link
Contributor Author

Thanks for pointing that out @AnaLoznianu. I believe that I've fixed all the issues now - all the tests pass locally. But i'm having trouble getting the CI checks to pass.

I've already spent over 2 hours on it this morning. Each CI run takes over 30 minutes. There are frequent random failures, especially Address file not found #939. But more recently, other failure modes have emerged.

  1. Invalid or unresponsive aquarius url http://172.15.0.5:5000/ https://github.com/oceanprotocol/ocean.py/actions/runs/3227265307/jobs/5281846034#step:7:263

  2. urllib3.exceptions.ReadTimeoutError: HTTPConnectionPool(host='127.0.0.1', port=8545): Read timed out. (read timeout=10) https://github.com/oceanprotocol/ocean.py/actions/runs/3222597344/jobs/5271829143#step:7:1115
    This one was particularly upsetting because it ran for over 3 hours instead of the typical 30 minutes.

@MantisClone
Copy link
Contributor Author

Hello @alexcos20 @calina-c @mariacarmina I believe this PR is ready for review.

@MantisClone MantisClone temporarily deployed to CC_REPORTER_ID October 13, 2022 18:32 Inactive
@MantisClone MantisClone temporarily deployed to CC_REPORTER_ID October 18, 2022 19:55 Inactive
@MantisClone
Copy link
Contributor Author

MantisClone commented Oct 28, 2022

@alexcos20 @calina-c @mariacarmina CC @AnaLoznianu
The tests are broken but they were working 9 days ago. I'd really love to get the changes in this PR merged in. Is this a realistic goal? Or should I stop maintaining this?

@MantisClone MantisClone temporarily deployed to CC_REPORTER_ID November 4, 2022 19:43 Inactive
@MantisClone
Copy link
Contributor Author

@alexcos20 CC @AnaLoznianu
Happily, it seems that the tests are passing again. Can you please review this at your soonest convenience?

@trentmc
Copy link
Member

trentmc commented Nov 28, 2022

This needs a README to show how to use. It can look similar to:

@MantisClone
Copy link
Contributor Author

MantisClone commented Dec 5, 2022

Hi @trentmc. Thanks for the comments. This PR has dropped off the end of my priority list and I don't have the bandwidth to continue pushing it forward.

I could hand-off this PR to someone on the core team by retargeting it to a development branch on the upstream repo instead of old-main). If this approach is desirable, please let me know what branch should be the target.

@trentmc
Copy link
Member

trentmc commented Jan 3, 2023

Closing, this was superseded by PR #1226, and merged to main in d6f6a90

@trentmc trentmc closed this Jan 3, 2023
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.

None yet

5 participants