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

feat: +Reverse engineering related logic #800

Closed
wants to merge 45 commits into from

Conversation

iorisa
Copy link
Collaborator

@iorisa iorisa commented Jan 27, 2024

Features

  1. Based on the source code, reverse engineer class diagrams, sequence diagrams, and use cases.
  2. There is still room for improvement in the logic for generating sequence diagrams of classes.
  3. The logic for symbol extraction only covers common Python usages, and there is still room for improvement.

莘权 马 and others added 2 commits January 29, 2024 23:13
@better629 better629 requested review from better629 and geekan January 31, 2024 07:15
Copy link
Owner

@geekan geekan left a comment

Choose a reason for hiding this comment

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

  1. should tests/data/graph_db/networkx.json be ignored?
  2. could you provide some readme or comments for the files?
  3. how to use the results as context, any design here?
  4. could we use vector search to search the symbols?

Copy link
Collaborator

@better629 better629 left a comment

Choose a reason for hiding this comment

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

Maybe here can be some docs/flows to better understand the logic inside actions.
And some docstring or example can be provided.

metagpt/actions/rebuild_sequence_view.py Show resolved Hide resolved
metagpt/actions/rebuild_sequence_view.py Show resolved Hide resolved
metagpt/actions/rebuild_sequence_view.py Show resolved Hide resolved
metagpt/repo_parser.py Outdated Show resolved Hide resolved
metagpt/repo_parser.py Show resolved Hide resolved
莘权 马 and others added 2 commits February 1, 2024 16:12
fixbug: add METAGPT model calc usage logic
metagpt/provider/azure_openai_api.py Outdated Show resolved Hide resolved
metagpt/provider/azure_openai_api.py Outdated Show resolved Hide resolved
metagpt/utils/token_counter.py Outdated Show resolved Hide resolved
metagpt/provider/azure_openai_api.py Outdated Show resolved Hide resolved
metagpt/utils/common.py Outdated Show resolved Hide resolved
metagpt/utils/token_counter.py Outdated Show resolved Hide resolved
@iorisa iorisa requested a review from geekan February 2, 2024 09:48
metagpt/utils/token_counter.py Outdated Show resolved Hide resolved
Copy link
Owner

@geekan geekan left a comment

Choose a reason for hiding this comment

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

  1. The generated annotations do not actually reduce the cost of reading, but in a sense increase the cost of reading
  2. Lack of tests and examples makes it difficult to understand if a feature is appropriate

metagpt/actions/rebuild_class_view.py Outdated Show resolved Hide resolved
metagpt/actions/rebuild_sequence_view.py Show resolved Hide resolved
metagpt/provider/base_llm.py Outdated Show resolved Hide resolved
metagpt/actions/rebuild_sequence_view.py Show resolved Hide resolved
metagpt/actions/rebuild_sequence_view.py Show resolved Hide resolved
metagpt/actions/rebuild_sequence_view.py Show resolved Hide resolved
metagpt/actions/rebuild_sequence_view.py Show resolved Hide resolved
metagpt/actions/rebuild_class_view.py Show resolved Hide resolved
metagpt/actions/rebuild_class_view.py Show resolved Hide resolved
metagpt/repo_parser.py Outdated Show resolved Hide resolved
Copy link
Owner

@geekan geekan left a comment

Choose a reason for hiding this comment

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

  1. Overall, there are a lot of useless comments generated and need to be cleared.
  2. There needs to be a clear design document explaining how to use it.

from metagpt.utils.di_graph_repository import DiGraphRepository
from metagpt.utils.graph_repository import GraphKeyword, GraphRepository


class RebuildClassView(Action):
"""
Reconstructs a graph repository about class diagram from a source code project.
Copy link
Owner

Choose a reason for hiding this comment

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

We need more explicit functional description and design. Its function description is vague. The run interface has no real input parameters, and all input and output parameters are implicit.

Reconstructs a graph repository about class diagram from a source code project.

Attributes:
graph_db (Optional[GraphRepository]): The optional graph repository.
Copy link
Owner

Choose a reason for hiding this comment

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

It's a simple duplication of variables, and since this class is actually pydantic.BaseModel, I don't think the annotation here is needed and should be explained by the fields themselves.

async def run(self, with_messages=None, format=config.prompt_schema):
"""
Implementation of `Action`'s `run` method.
Copy link
Owner

Choose a reason for hiding this comment

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

Class is not a current specification and is recommended to be removed

Implementation of `Action`'s `run` method.

Args:
with_messages (Optional[Type]): An optional argument specifying messages to react to.
Copy link
Owner

Choose a reason for hiding this comment

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

This value requires appropriate type annotation and comments, rather than LLM generating text with no additional information

# - gpt-4-turbo-vision: "gpt-4-vision-preview"
# - gpt-4 8k: "gpt-4"
# See for more: https://azure.microsoft.com/en-us/pricing/details/cognitive-services/openai-service/
# pricing_plan: "gpt-4-turbo-preview"
Copy link
Owner

Choose a reason for hiding this comment

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

It requires very clear annotations to explain its use and scenarios to reduce the cost of understanding.

None

Raises:
SomeException: Describe any exceptions that might be raised during the saving process.
Copy link
Owner

Choose a reason for hiding this comment

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

These comments, if they have no actual meaning, should be deleted. Please check other sections

Note:
The JSON file should contain node-link data compatible with the format produced by the 'json' method.

"""
data = await aread(filename=pathname, encoding="utf-8")
m = json.loads(data)
self._repo = networkx.node_link_graph(m)

@staticmethod
async def load_from(pathname: str | Path) -> GraphRepository:
Copy link
Owner

Choose a reason for hiding this comment

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

Generally speaking, this is more appropriately named from_json_file

# Retrieves the root directory path for the graph repository files.

Note:
This property provides the directory path where graph repository files are saved or loaded.
Copy link
Owner

Choose a reason for hiding this comment

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

251-263 seems to be reduced to one or three lines.

Note:
This property provides the full path, including the filename, to the graph repository file.

"""
Copy link
Owner

Choose a reason for hiding this comment

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

268-280 seems to be reduced to one or three lines.

Note:
This property provides direct access to the networkx.DiGraph instance used by the graph repository.

"""
Copy link
Owner

Choose a reason for hiding this comment

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

285-298 seems to be reduced to one or three lines.

@iorisa iorisa closed this Mar 5, 2024
@iorisa iorisa deleted the feature/rebuild branch March 5, 2024 02:08
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.

5 participants