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

Feature/agents #1

Merged
merged 10 commits into from
Nov 4, 2024
Merged

Feature/agents #1

merged 10 commits into from
Nov 4, 2024

Conversation

monoxgas
Copy link
Contributor

@monoxgas monoxgas commented Oct 24, 2024

This merges in an early take on agent management used for demos. Figure we can work through elements of the refactor and make sure I didn't over-simplify the auth core.

  • Added some user information like email, username, and api-key to the config. I wonder if this is excessive state to store - I worry about keeping it in sync.
  • Reworked authentication slightly to use just cookies and let the server refresh for us.
    • Re-used that great onexit register to pull the latest cookies back out every time we exit (Feels pretty messy to me right now, might need some work)
    • Not doing any opportunistic/manual refreshes -> currently thinking of the refresh command as refreshing user information
    • I did leave in the JWT parsing so we can check a refresh token before we use it and avoid causing a 401 anyways - but missing one request isn't the worst thing.
    • If we ever get a 401 and we were sending cookies, we can assume we need to login again
  • Added a decorator to wrap all the cli functions, catch errors, and print them cleanly - just pulling out logic that was explicit everywhere into one place
  • I started adding some models to type out the api functions, but this feels like a weak step towards the final outcome of just generating them. It helps with code sanity (type hinting), but doesn't help with stability because they could get out of sync just as easily
  • I'm "kind-of?" liking the format.py strategy for the agent where formatting all objects is abstracted somewhere central - loving rich btw - but maybe we should pull this out closer to a dedicated "Schemas" section when we have it. Essentially you are defining for each schema model, how it would be rendered.
  • Move to YAML BABY YEAH! (should we support both here? I have no idea how to do that)

pyproject.toml Show resolved Hide resolved
dreadnode_cli/utils.py Show resolved Hide resolved
dreadnode_cli/api.py Show resolved Hide resolved
dreadnode_cli/api.py Show resolved Hide resolved
dreadnode_cli/api.py Outdated Show resolved Hide resolved
env_server = os.getenv("DREADNODE_SERVER")
if env_server:
server = env_server
if not server:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would love a double check on this logic - want to make sure we're properly handling all the cases for "is their an existing profile?" "did the user give us a server?" "do we know about a server already?"

Copy link
Contributor

Choose a reason for hiding this comment

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

@monoxgas i had a doubt while reading the multi profile logic initially: what's the advantage of this approach versus, say, having just a unified --profile path/to/dreadnode.json argument? isn't manually switching via dreadnode CLI just more complex and less intuitive than simply using a different profile if needed and defaulting to ~/.dreadnode/config.json? that'd allow us to get rid of the complexity and all edge cases (really not a fan of UserConfig._update_active)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me sleep on this, It's a good point. I think there is something to be said about how other command line tools carry context (AWS, Kubectl). The former just asks for a profile argument to switch, the other does carry the idea of an active context at any given moment.

Also worth considering that carrying context for multiple servers is extremely rare for standard users ATM.

@monoxgas monoxgas requested review from evilsocket and removed request for evilsocket October 24, 2024 15:13
@monoxgas monoxgas marked this pull request as draft October 24, 2024 15:19
@monoxgas monoxgas marked this pull request as ready for review October 24, 2024 15:20
@@ -5,7 +5,7 @@ Dreadnode command line interface.
### With Poetry:

```bash
poetry install
poetry install
Copy link
Contributor

Choose a reason for hiding this comment

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

@monoxgas is there anything we can add here to have the dreadnode CLI in $PATH even outside the poetry shell? not very familiar with poetry but it seems that huggingface_hub is using a setup.py for this

client = docker.from_env()


# TODO: Poor form having a fixed list of registries IMO
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 just check for any dev- or staging- prefixes in config.url and prepend whatever we find to the registry.dreadnode.io ... not great but would make things a bit neater.


def build(directory: str | pathlib.Path) -> Image:
id: str | None = None
for item in client.api.build(path=str(directory), platform="linux/amd64", decode=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

if using the local registry, this will probably fail if ARM and buildx is not installed on the host

@@ -1,4 +1,4 @@
FROM python:3.10-slim
FROM amd64/python:3.10-slim
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not force architectures in Dockerfiles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Let me align this with the other architecture considerations above as well.

print("Chat:", chat.conversation)

try:
response = httpx.get("http://web")
Copy link
Contributor

Choose a reason for hiding this comment

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

missing import httpx?

dreadnode_cli/api.py Show resolved Hide resolved
@@ -23,38 +22,33 @@ def format_difficulty(difficulty: str) -> str:

# TODO: add sorting and filtering
Copy link
Contributor

Choose a reason for hiding this comment

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

@monoxgas do we actually care for the CLI?


challenges = asyncio.run(client.list_challenges())
table = Table(box=box.ROUNDED)
Copy link
Contributor

Choose a reason for hiding this comment

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

i kind of like the "format_" approach, very MVC, can I move this there?

env_server = os.getenv("DREADNODE_SERVER")
if env_server:
server = env_server
if not server:
Copy link
Contributor

Choose a reason for hiding this comment

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

@monoxgas i had a doubt while reading the multi profile logic initially: what's the advantage of this approach versus, say, having just a unified --profile path/to/dreadnode.json argument? isn't manually switching via dreadnode CLI just more complex and less intuitive than simply using a different profile if needed and defaulting to ~/.dreadnode/config.json? that'd allow us to get rid of the complexity and all edge cases (really not a fan of UserConfig._update_active)

print(":white_check_mark: authentication successful")
except Exception as e:
print(f":cross_mark: {e}")
webbrowser.open(verification_url)
Copy link
Contributor

Choose a reason for hiding this comment

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

can raise and interrupt the authentication flow

@evilsocket evilsocket merged commit ba76053 into main Nov 4, 2024
3 checks passed
@evilsocket evilsocket deleted the feature/agents branch November 4, 2024 10:39
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