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

Update newest (no deep diff) #449

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from
Open

Update newest (no deep diff) #449

wants to merge 22 commits into from

Conversation

duboyal
Copy link

@duboyal duboyal commented Apr 1, 2024

Description

Changes

Known Issues

Notes

Checklist

  • My name is on the list of contributors (CONTRIBUTORS.md) in the pull request source branch.
  • I have updated the documentation to reflect my changes.
  • My code changes have been verified by automated tests and pass all relevant test scenarios.

Copy link

trunk-io bot commented Apr 1, 2024

Merging to develop in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

@duboyal duboyal changed the base branch from main to develop April 1, 2024 15:03
print(proj)

print("------------\n1111111")
print(type(proj))
Copy link
Author

Choose a reason for hiding this comment

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

@InnocentBug Ludwig, if you have a moment do you mind telling me why this might load in a string?

also my work flow and logic in this test , is to ensure that I can save the node as just a "creation" step , and then be able to reload the node back in ( via using "load nodes from json") ,

then the idea to edit it , save it , then load it again ensuring the changes are indeed saved,
but the return value should be a node object right ? this is also what the function docs say and what I remember in my past work. I made this branch off of the current development and that function has not been touched by the changes in this PR so I'm a bit perplexed though its probably something simple , so I'm wondering what your thoughts are

Copy link
Collaborator

Choose a reason for hiding this comment

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

It does load a string if it thinks that it isn't a node.
(Or valid JSON)

print(proj)

print("------------\n1111111")
print(type(proj))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It does load a string if it thinks that it isn't a node.
(Or valid JSON)

proj_json = proj0.get_json().json
cript_api.save_new(proj0)

proj = cript.load_nodes_from_json(nodes_json=json.dumps(proj_json))
Copy link
Collaborator

Choose a reason for hiding this comment

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

also be careful here, if you load an existing project with the same UUID.
It will give you the same nodes as you have before.

You can't have different python nodes with the same UUID, except if you use the _use_cache argument of the load function.

@InnocentBug
Copy link
Collaborator

I just realized, that we may get a number of false positive equal_shallow and equal_deep calls.
The API adds info to nodes, like created_at etc. and these don't actually mean a diff, but will trigger a patch in the current design.

I don't think that is an issue, but something to keep in mind, and maybe for optimization later.

data = new_node.get_json().json
response = self._capsule_request(url_path="/project/", method="POST", data=data)
print("----700----")
print(response.json())
Copy link
Author

@duboyal duboyal Apr 2, 2024

Choose a reason for hiding this comment

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

@InnocentBug

Hey Ludwig
this is giving me the "bad uuid" for a project when I run "test inventory" (I mean when I run the whole test file, its seeming to give an issue on one of the integrations I'm a little unfamiliar with the work flow)

I get this out put

--------900---------
project
----700----
{'code': 400, 'data': None, 'error': 'Bad uuid: 9aa4b9e7-1b7c-4c76-9602-2c980bc4298a provided'}
Screenshot 2024-04-02 at 5 43 51 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is probably the same issue that we discussed before.

The API requires all materials to be known before an inventory can be saved.
But in our tests, we have inventories with new materials in them.

So, you get a bad UUID, when you try to save one.

We will have to redesign the fixtures, such that they contain known materials.

Copy link
Collaborator

Choose a reason for hiding this comment

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

for now, we can ignore it, I guess.

Or better, change the inventories, such that they contain no or known materials.

But we also have to think about the user, parse this error, and give a good error message, when this happens.

print(_no_condense_uuid)
def _internal_save(self, new_node: PrimaryBaseNode, preknown_uid: str, _no_condense_uuid: bool) -> None:
"""
NOTE: for Ludwig
Copy link
Author

Choose a reason for hiding this comment

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

@InnocentBug

Ludwig , this is still Work In Progress, I think I fleshed out most of the skeleton but where to put and use the "uid map" is causing me to scratch my head a bit. I left a pretty detailed note in the docstring here. do you mind taking a a glance at it ? (the note?) and let me know your thoughts?

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.

2 participants