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

use relative imports #21

Closed
wants to merge 8 commits into from

Conversation

pmp-p
Copy link
Contributor

@pmp-p pmp-p commented Jul 20, 2022

and add wasm compatibility

@KennedyRichard
Copy link
Collaborator

Thank you for the pull request. I'll review the changes in a few days or next week and let you know in case I have any questions.

@KennedyRichard KennedyRichard self-requested a review July 20, 2022 19:46
@KennedyRichard KennedyRichard changed the base branch from main to pr-integration September 1, 2022 15:08
@KennedyRichard
Copy link
Collaborator

Hello, Mr. Peny,

I was finally able to finish working on the scheduled tasks and solved the pending issues as well. Thank you for your patience and again, sorry for the delay.

I just started reviewing your changes.

@KennedyRichard
Copy link
Collaborator

I didn't finish reviewing all files, but after quickly scanning many of the files changed I'd like to share my thoughts on the changes and listen to your feedback on all of this.

I think I can divide the changes into the following categories:

  • converting absolute imports into relative ones;
  • removing harmful needless whitespace (fixing leading and trailing spaces);
  • converting the mainloop and other minor calls/imports for compatibility with pygame-web, including running the app asynchronously;

As I said before on discord, I'd like to keep the web-related version of nodezator separate from the original desktop-only code. I already explained the reasoning in the discord conversation I just linked, but I'll leave it quoted below for easy reference:

I'm inclined to merge the changes into a side branch and turn it into a new repository to be maintained in parallel, though, something like a web version for Nodezator, separate from the Nodezator desktop app. This is so because keeping them as a single app will certainly pose limitations for both platforms, web and desktop. To be more precise, there's just too much stuff planned for Nodezator desktop that I doubt can be smoothly or even at all implemented on the web. Conversely, I'm sure the web version must be able to take advantage of stuff which would not make sense to be on the desktop app.

To be completely honest, I don't think there is a satisfactory solution to either scenario. Here are the trade-offs I see for each scenario.

single app for web and desktop one app for web and one for desktop
advantage all code in a single place each app only need to worry about its own environment and thus can fully explore its strengths
disadvantage every time a change is not compatible with one of the environments we'll have to either discard it or wrap the code in if/else clauses having to develop/manage 02 apps

However, taking both scenarios into account, I consider having a different app for each environment to be the scenario which leads to the healthiest/better designed and easier to maintain codebase (for both desktop and web versions).

In addition to all of this, in our brief conversation on discord that led to this PR, we actually talked only about the relative imports stuff. I'm not mentioning this to imply that unexpected/undiscussed changes are not welcome. I've even already approved PRs in nodezator with changes made without prior notice (including one PR from you). I'm only mentioning this to explain why I only now I'm sharing my thoughts on such changes. It is just that I didn't know you'd be making those changes in the PR and only now I had the time to take a careful look.

I hope, regardless of whether you agree or not, I was able to make my reasons clear. I reiterate that no change needs my prior knowledge to be made and all changes are welcome into nodezator, except that, as nodezator's maintainer, sometimes I need to intervene to guarantee nodezator is advancing towards what I consider to be a healthier design.

@KennedyRichard
Copy link
Collaborator

And all of this leads us to my next point:

If we were to discard the changes related to web-compatibility, we'd only be left with the changes regarding (1) conversion of absolute imports into relative ones and (2) removal of harmful needless whitespace. Those are both very desired and welcome changes, but there are some catches: since your PRs, a lot of code was added to nodezator, and thus a lot of conflicts exists as well as code that, despite not having conflicts, need updating.

So, for those changes I have a proposal, if you agree: I'd like to reject the PRs entirely and instead perform the changes programmatically and, since you did most of the work, commit the changes attributing the commits to you as the author. I'm pretty convinced I can come up with a small Python utility to correctly identify the local imports and convert them from absolute into relative ones.

For the removal of harmful needless whitespace, I'm pretty sure a code linter can do the trick. I've been willing to use a linter in nodezator codebase since a long time ago anyway.

I don't even need to throw away the PRs at first. I can try to implement the changes I just mentioned locally and, only if it works and after testing the changes, finally reject the PRs and commit the work with your authorship.

This would solve 02 problems:

  • save you from all the extra manual work needed to convert the additional code/modules;
  • since the changes would be applied programmatically, they would be less error-prone;

And, as I already said, you'd still retain the deserved recognition of your work in this PR by having authorship of the changes.

@KennedyRichard
Copy link
Collaborator

KennedyRichard commented Sep 2, 2022

I can come up with a small Python utility to correctly identify the local imports and convert them from absolute into relative ones.

Thankfully I was able to create the utility I proposed in my last message. I even had time to upload it to pypi. It's called abs2rel.

I already tested it with the current state of nodezator and it was able to convert all but a single import statement from all the hundreds of python files in a single command. That'll save us a lot of time.

I also have other unpublished packages whose imports I wanted to turn into relative ones before their release, so I had been thinking about the algorithm for quite some time.

@pmp-p
Copy link
Contributor Author

pmp-p commented Sep 3, 2022

sure close/adapt/whatever git magic that PR and do it programatically it's safer

Now for the conversation i think maintening two(desktop+web) or three(+wasi for vsc) or four(+android) version is risky for the future and adoption so here's my position :

CPython is versatile enough to handle a lot of platforms. and android/web/wasi are not even real yet => this gives quite a lot of time to :

1 ) Learn - and TEST - how to do things on web : who would/could handle that task anyway - right now - since the concept is so new ? i do asm.js/wasm since 2016 and i still feel like a headless chicken running around and things change everyday ( e.g. : there's threads and 64bits now that weren't really there this past months ).

2 ) Choose carrefully the best practices to do (1) applied to something now very different from games (pygame/panda3d/harfang3d) and scientific stack notebooks (pyodide).

3 ) Apply the best solutions one patch at a time to nodezator and make it a technologic show.

if imports are cleaned , and code base sanitized by black i see no problem to transition smoothly without the burden of multiple branches

PS: for (1) i'd say if code is robust enough to handle web, then it is - definitively - better quality code for desktop ( no threads in python please ) and better user experience ( async is user reactive).

@KennedyRichard
Copy link
Collaborator

Despite my reservations I've been thinking about all of this and, yeah, I'm convinced that your point is worth pursuing, Mr. Peny.

I guess I'm just trying not to be overly optimistic about something that people doesn't even know much about. As you said yourself everything is much recent.

I'm still of the mind that a lot of software out there try to do/be many things and in the process harm their own growth, stability and health, but in this particular case it may indeed be worth to keep nodezator both desktop and web-ready.

With all of this in mind I think this will be the strategy I'll be following:

  • I'll reject your PRs and perform the absolute-to-relative import conversion on nodezator;
  • I'll pursue the path of making nodezator web-ready, but we'll do it gradually:
    • First, I'll postpone the implementation a bit, until I can research and write about it in detail and present it for discussion;
    • Only then, after gathering feedback and discussing it enough, we'll proceed with the needed changes;

I count on your help with the discussions/implementations needed in the future, please, since this is not my area of expertise and to make matters even more complex, besides already being quite the complex piece of software, nodezator is not even in its final form as there are many stuff still to be added to it. For instance, just to name a few:

  • changing its graphical backend to use pygame+opengl rather than the current pure pygame approach;
  • implementing some form or concurrency/parallelism on graph execution so nodes which are not dependent on each other don't need to be executed sequentially;
  • adding branching (if/elif/match/else) and looping to graph execution;

I've actually been researching concurrency/parallelism in Python for many months through many books, even before nodezator's release. At the time, I concluded that what I need to do has not been attempted yet, or at least no in the scale/scope I desire. Since the software had not been released yet, I also lacked the perspective of knowledgeable people on the matter. Because of that, I decided back then to keep nodezator a sequential app until I'm 100% on how to approach concurrency/parallelism. Now that nodezator is release, we can finally approach the problem in a rational well-thought manner, but I think we need time to precisely specify the requirements of the changes we need before compromising with any specific solution.

For the next few weeks I'll be working on Mr. Císař's proposal, then I'll proceed to add branching to nodezator. Therefore, I expect to be revisiting this matter of making nodezator web-ready by the middle or end of October. I again, ask for your patience until then.

I'm just trying to be careful about which changes should be brought to nodezator. Even Python itself which has a lot of maintainers/expert eyes on it needs PEPs and time to review them. So with nodezator it would not be different, specially since we don't have many people involved.

Thank you for being understanding, proactive and for your selfless contributions to this project.


Also, about applying black formatting to the codebase, it will be one of my next steps. I'll also perform some linting to the code, since I'm sure it must have a lot of problems, not only bad formatting. Stuff like unused imports etc.

@KennedyRichard
Copy link
Collaborator

Closing this PR and #22 as well as agreed in this conversation.

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