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.

Sign up for GitHub

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

Jump to bottom

Update for lang-graph rewrite (maltoolbox 0.2) #89

Merged
merged 9 commits into from
Jan 27, 2025
Merged

Update for lang-graph rewrite (maltoolbox 0.2) #89

merged 9 commits into from
Jan 27, 2025

Conversation

nkakouros
Copy link
Collaborator

Fixes to make the simulator work with the lang-graph rewrite of mal-toolbox.

@nkakouros nkakouros requested a review from andrewbwm January 13, 2025 18:17
@mrkickling
Copy link
Collaborator

mrkickling commented Jan 27, 2025 •
edited
Loading

Did I do double work again (#94)..

But some notes:

  • Your solution is mostly cleaner than mine
    • But I think the association/step types would need to be unique (set instead of list).
  • I am a bit unsure about the class.name.removeprefix, we should have a cleaner way to fetch the full_name.. My solution seems safer but also uglier. So I am not sure about that.
  • The observation space also needs to be updated (part of my PR)
  • The model files need to be updated (part of my PR)

I could make a PR into your PR, keeping the parts I like in your solution but adding the required changes that are only in mine, so it is all in one place?

@nkakouros
Copy link
Collaborator Author

Where can i find your solution?

@nkakouros
Copy link
Collaborator Author

Where can I learn to read?

@mrkickling
Copy link
Collaborator

Where can I learn to read?

Well, I might also take that course so I read the name of your PR next time before I make my own:)

@nkakouros
Copy link
Collaborator Author 

  * But I think the association/step types would need to be unique (set instead of list).

I agree.

* I am a bit unsure about the **class**.**name**.removeprefix, we should have a cleaner way to fetch the full_name.. My solution seems safer but also uglier. So I am not sure about that.

Actually is this still needed? I don't think Association_ is being prefixed anymore, which means that this can be simplified to just assoc.__class__.__name__ which is the idiomatic way of getting the class name which is also what we need (as long as we still use a model asset factory).

* The observation space also needs to be updated (part of my PR)

Can you pull that part here? You can do git checkout -p <your-branch> -- path/to/file/Is/it/model.py?

* The model files need to be updated (part of my PR)

Can you pull these changes here?

I could make a PR into your PR, keeping the parts I like in your solution but adding the required changes that are only in mine, so it is all in one place?

Yes, makes sense. I think you can push directly here.

@mrkickling
Copy link
Collaborator

Actually is this still needed? I don't think Association_ is being prefixed anymore, which means that this can be simplified to just assoc.__class__.__name__ which is the idiomatic way of getting the class name which is also what we need (as long as we still use a model asset factory).

I think it is still prepends 'Association_' in the lang classes factory. But that will soon be forgotten.

I will try to put the valid changes from my PR into this PR.

@mrkickling mrkickling changed the title Update for lang-graph rewrite Update for lang-graph rewrite (maltoolbox 0.2) Jan 27, 2025
@mrkickling
Copy link
Collaborator

Let me know what you think

nkakouros
nkakouros commented Jan 27, 2025
View reviewed changes
malsim/scenario.py Outdated Show resolved Hide resolved
nkakouros
nkakouros commented Jan 27, 2025
View reviewed changes
malsim/sims/mal_simulator.py
Comment on lines 542 to 567
self._index_to_id = [n.id for n in self.attack_graph.nodes]
self._index_to_full_name = [n.full_name
for n in self.attack_graph.nodes]
self._index_to_asset_type = [n.name for n in self.lang_graph.assets]
self._index_to_step_name = [n.asset.name + ":" + n.name
for n in self.lang_graph.attack_steps]
self._index_to_model_asset_id = [int(asset.id) for asset in \
self.attack_graph.model.assets]
self._index_to_model_assoc_type = [assoc.name + '_' + \
assoc.left_field.asset.name + '_' + \
assoc.right_field.asset.name \
for assoc in self.lang_graph.associations]
self._index_to_full_name = (
[n.full_name for n in self.attack_graph.nodes]
)
self._index_to_asset_type = (
[n.name for n in self.lang_graph.assets.values()]
)

unique_step_type_names = {
n.full_name
for asset in self.lang_graph.assets.values()
for n in asset.attack_steps.values()
}
self._index_to_step_name = list(unique_step_type_names)

self._index_to_model_asset_id = (
[int(asset.id) for asset in self.attack_graph.model.assets]
)

unique_assoc_type_names = {
assoc.full_name
for asset in self.lang_graph.assets.values()
for assoc in asset.associations.values()
}
self._index_to_model_assoc_type = list(unique_assoc_type_names)

# Lookup dicts attribute to index
self._id_to_index = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most of these will be possible to drop once mal-lang/mal-toolbox#105 is merged.

@mrkickling mrkickling self-requested a review January 27, 2025 14:17
@mrkickling mrkickling merged commit 111b572 into main Jan 27, 2025
6 checks passed
@nkakouros nkakouros deleted the lang-graph-rewrite branch January 29, 2025 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrkickling mrkickling mrkickling approved these changes

@andrewbwm andrewbwm andrewbwm approved these changes

Assignees

@mrkickling mrkickling

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nkakouros @mrkickling @andrewbwm