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

Add Api Configuration #1064

Merged
merged 10 commits into from
Dec 19, 2022
Merged

Add Api Configuration #1064

merged 10 commits into from
Dec 19, 2022

Conversation

lucekdudek
Copy link
Contributor

@lucekdudek lucekdudek commented Dec 2, 2022

  • add api_config pydoc
  • improve deprecate comments
  • move deafault values from all over the code to Api Config class
  • add tests for ApiConfig

@lucekdudek lucekdudek self-assigned this Dec 2, 2022
Copy link
Contributor

@shadeofblue shadeofblue left a comment

Choose a reason for hiding this comment

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

as previously discussed, this is a good direction but please see my comments,

especially about separation of subnet-tag and payment variables from the ApiConfig ... they don't have too much in common and should not be put in the same container...

most importantly neither the subnet, nor the payment parameters are required to instantiate any of the APIs

tests/conftest.py Outdated Show resolved Hide resolved
tests/engine/test_debit_note_intervals.py Outdated Show resolved Hide resolved
yapapi/config.py Outdated Show resolved Hide resolved
tests/engine/test_engine.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/services/test_cluster.py Outdated Show resolved Hide resolved
yapapi/config.py Outdated Show resolved Hide resolved
yapapi/engine.py Show resolved Hide resolved
yapapi/golem.py Outdated Show resolved Hide resolved
yapapi/config.py Outdated Show resolved Hide resolved
@lucekdudek lucekdudek marked this pull request as ready for review December 8, 2022 15:20
@lucekdudek lucekdudek requested a review from a team December 8, 2022 15:20
examples/low-level-api/list-offers.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/factories/golem.py Show resolved Hide resolved
tests/engine/test_engine.py Outdated Show resolved Hide resolved
yapapi/golem.py Outdated Show resolved Hide resolved
yapapi/golem.py Show resolved Hide resolved
yapapi/config.py Outdated Show resolved Hide resolved
yapapi/config.py Outdated Show resolved Hide resolved


@dataclass
class ApiConfig:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, sorry if this is OT, but.

Will the ApiConfig work separate from all of the other yapapi stuff? E.g. can I do in my (otherwise non-yapapi) code do from yapapi.config import ApiConfig and use it?

If yes, that would be nice :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not intended but yes.

IDK if you would want to add yapapi dependency just to use a dataclass 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, maybe for development purposes at least?
Also, that might not be the only imported thing:

(gc) jbetley:~/golem-core-python {jb/yacat-v2} $ grep -rIh 'from yapapi' golem_core/
from yapapi.payload import Payload as YapapiPayload, vm
from yapapi import rest
from yapapi.engine import DEFAULT_DRIVER, DEFAULT_NETWORK, DEFAULT_SUBNET
from yapapi.props.builder import DemandBuilder
from yapapi import props
from yapapi.storage import Destination, Source
from yapapi.storage.gftp import GftpProvider
from yapapi.engine import NoPaymentAccountError
from yapapi.log import _YagnaDatetimeFormatter
from yapapi.engine import DEFAULT_NETWORK, DEFAULT_DRIVER, DEFAULT_SUBNET
from yapapi.props.base import constraint
from yapapi.props import inf

:)

Copy link
Contributor

@shadeofblue shadeofblue left a comment

Choose a reason for hiding this comment

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

lgtm :)

btw, please add budget to GolemFactory so that it doesn't need to be provided in each case...

@shadeofblue shadeofblue merged commit 8cf9efd into master Dec 19, 2022
@shadeofblue shadeofblue deleted the ld/yagna-api-url branch December 19, 2022 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants