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

Implement force_identicial_write #260

Conversation

nathanjmcdougall
Copy link
Contributor

@nathanjmcdougall nathanjmcdougall commented Jul 20, 2024

To resolve #241.

I struggled for a while to realize that prepare_pin_version deletes the old pin via version_setup for unversioned boards. To deal with this, I have pre-emptively retrieved the meta object before it is deleted, and then used that to compare the hash.

I think part of the issue is perhaps that version_setup doesn't belong in pins/versions.py - I am of the view that it is currently too decontextualized and would be better closer to the place where it is invoked, in prepare_pin_version. A docstring would help too.

It's worth noting that this is a breaking change - as a result some of the existing tests needed modifying to opt-in to the old behaviour.

I'm not sure whether the READMEs need any updates. I took a read but I think it would be distracting to detail this feature.

@nathanjmcdougall
Copy link
Contributor Author

nathanjmcdougall commented Jul 22, 2024

I've realized the best place to add docs is probably the get_started.qmd notebook.

However, it's hard to demonstrate the functionality without encountering the same issue as described in #258.

Perhaps the solution is created= arguments, but this muddies the illustration. Another option is to separate the calls between separate cells and hope the user doesn't do Run Above or a similar high-frequency cell execution.

For now, I have used time.sleep.

@isabelizimm
Copy link
Collaborator

isabelizimm commented Jul 23, 2024

just wanted to say that I've seen this and will give it a closer look in the next few days (edit: got swept away with positconf prep, but will be getting back into the swing of things here shortly)! and to say I've never seen pytest.CaptureFixture[str] before and am VERY EXCITED to learn about its existence 👀

@nathanjmcdougall
Copy link
Contributor Author

I hope the conference went well!

I think the test that's failing needs to change based on the new default behaviour, I'll take a look now.

@nathanjmcdougall nathanjmcdougall force-pushed the feature/241-implement-force-identical-write branch from 4c31b9b to c95ecea Compare August 27, 2024 22:22
@isabelizimm
Copy link
Collaborator

It did! It's one of my favorites (not that I am biased or anything 😆)

I think the test that's failing needs to change

++ I can kick off the full set of tests again in a moment here to pick up your new commits!

@isabelizimm
Copy link
Collaborator

I struggled for a while to realize that prepare_pin_version deletes the old pin via version_setup for unversioned boards.

Late comment, but some history here is that Posit Connect cannot delete the most recent pin. IIRC that is in place since pins actually has to create a second pin and then delete the older pin for an unversioned board. The placement is also in parity with the R side; that being said, parity is not something hugely important to maintain if it doesn't make sense but is a subtle factor sometimes. I plan to leave it as is for now, but it is good to note the internal ergonomics! 💯

Copy link
Collaborator

@isabelizimm isabelizimm left a comment

Choose a reason for hiding this comment

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

did some small update around the tests, but this looks good to me!

@@ -248,8 +250,16 @@ def _pin_store(

pin_name = self.path_to_pin(name)

# Pre-emptively fetch the most recent pin's meta if it exists - this is used
# for the force_identical_write check
abort_if_identical = not force_identical_write and self.pin_exists(name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this may make writing pins slightly slower by making a call to the pin in self.pin_exists(). I think it's right move here (and I don't see a significant slowdown in tests at all), but just noting it if other calls get added and things start to feel sluggish

@isabelizimm isabelizimm merged commit 85e8cc3 into rstudio:main Sep 17, 2024
27 checks passed
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.

pin only new data
2 participants