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

Setup #61

Merged
merged 32 commits into from
Apr 7, 2021
Merged

Setup #61

merged 32 commits into from
Apr 7, 2021

Conversation

jkbecker
Copy link
Contributor

@jkbecker jkbecker commented Mar 31, 2021

A setuptools-based installation script as well as the required re-structuring of the entire project.

See updated README.md for the new installation instructions once this is in effect.

@jkbecker
Copy link
Contributor Author

jkbecker commented Mar 31, 2021

Just some mental notes of what doesn't work yet

Copy link
Collaborator

@altendky altendky left a comment

Choose a reason for hiding this comment

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

Overall, it seems a pretty tidy transition as it should be. Thanks! Don't let the quantity of minor comments etc express otherwise. And yeah, in work etc. I prefer to share early and often in case there are significant issues that should be addressed early. Handily, I don't think there are any here.

I'll note here that this includes the changes in #60. If this is merged first it is accepting both changes. I don't have a problem with #60, just pointing it out.

MANIFEST.in is for the sdist, IIRC. If you specify include_package_data = True then it will make it into the wheel as well and thus the installation. IIRC... I think this may also replace [options.data_files]. But it's bin awhile since I've fiddled with this and it's one of the areas where there are many ways to do it and most have gotchas.

Maybe https://pypi.org/project/appdirs/ and a CLI override option for config file locations? Though this strikes me as maybe a separate issue unless installation breaks the existing functionality? I should go see what it is.

Maybe src/plotman/plotman.py should be core.py or something? Module paths like plotman.plotman strike me as a bit odd.

I would avoid encouraging people to activate the environment when running. There's plenty of room for personal preference on whether to activate or not but from the perspective of supporting people it ends up adding hidden global state in $PATH that you have to debug. Whereas if people send you a traceback with venv/bin/plotman then you know they are running the plotman in the env.

Looking good for a first share. I think this is one of the first things I poked @ericaltendorf about getting done with plotman. Good to see it happening!

setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
src/plotman/plotman.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@altendky
Copy link
Collaborator

altendky commented Apr 3, 2021

@jkbecker, for getting the config from the 'right place', I think that can be a separate PR? This is already a restructure so keeping it focused is more valuable than normal. Also keeping it from getting held up on other changes is good. Unless I'm misunderstanding and the config location is particularly affected by this? From a quick search it seems to just be looking in cwd right now.

@jkbecker
Copy link
Contributor Author

jkbecker commented Apr 4, 2021

The main reason I wanted to couple those topics is: Once plotman is properly installed, it can be launched with the plotman command from anywhere, but the config.yaml references in the code are dependent on the current working directory. Essentially this will break if you're in the wrong directory, and as soon as you have a globally available command, that's not intuitive.

That said, considering I'll probably work on that next anyways, we could separate it as well?

There are some considerations when it comes to "correct" config file location handling.

  • Typically you'd want the default location to be in appdir's user_config_dir
  • To satisfy existing users we may want to attempt to read the config.yaml in the current directory with higher priority (?) (if it is in fact a plotman config.yaml - could be dangerous to just ingest any file with that name that happens to be in the working directory...
  • With highest priority, plotman should also respect path overloading via a variable such as PLOTMAN_CONFIG=~/config.yaml plotman interactive if desired

Does this make sense? Am I missing a special case? Happy to deal with this in a separate PR if it makes the individual contributions more manageable, either way is fine for me.

@altendky
Copy link
Collaborator

altendky commented Apr 4, 2021

Agreed on all points. I'm not sure if you said this indirectly or not, but config.yaml may just be a bad name at this point. Certainly understandable how it came to be though. :] I'd tend to separate the two just to avoid holding one up on the other and to have simpler to track reviews. Won't be making a fuss either way though.

@jkbecker jkbecker marked this pull request as ready for review April 4, 2021 21:15
Copy link
Collaborator

@altendky altendky left a comment

Choose a reason for hiding this comment

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

I'll commit the minor tweaks tomorrow after reviewing my own review at a more appropriate hour. @ericaltendorf do you want to discuss the effects of this on users before merging to develop? No more launching as ./plotman.py and such. Or, is the addition to the README sufficient?

Thanks again @jkbecker, definitely happy to have this change. :]

setup.cfg Outdated Show resolved Hide resolved
src/plotman/plotman.py Outdated Show resolved Hide resolved
src/plotman/plotman.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/plotman/job.py Outdated Show resolved Hide resolved
@ericaltendorf
Copy link
Owner

I think the addition to the README should be sufficient, along with an announce on the plotman keybase channel. I guess improving the config.yaml situation will be a separate bit of work?

(FWIW what I've been doing so far is creating a sibling directory (e.g., plotman-run/) to the plotman/ git local repo, putting a symlink to ../plotman/plotman.py in there and a copy of config.yaml that I modify with my local settings.)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
such as with --editable installation
@altendky
Copy link
Collaborator

altendky commented Apr 6, 2021

I guess I may as well draft the keybase message here. Suggestions and corrections are of course appreciated. Or, you could handle this if you want, @jkbecker. :]


Hi all. @jkbecker has kindly reworked the project structure of plotman for us so that it will be a properly installable Python package. This is being merged into the development branch. If you are using the main branch, this does not yet affect you. The new setup process has been documented in the readme. While this specific change does not change how plotman finds the config.yaml it would be understandable for some to be unsure where to put it. plotman still looks for config.yaml in the current working directory. This is the directory you are in in your shell when you run the program. You can keep your config wherever you wish.

Now that I've mentioned how the config.yaml is found presently, I'll go ahead and note that there has been some talk of changing that. The topic is ticketed as #84.

https://github.com/ericaltendorf/plotman#installation
#84

@jkbecker
Copy link
Contributor Author

jkbecker commented Apr 6, 2021

Looking good @altendky, thank you! I prefer not to announce it myself, as I'm not previously associated with the project. Don't want this to look like an intrusion ;)

❗ One detail just came to mind: If people just read the README on GitHub and pip install as suggested, they will actually have no config.yaml on their system at all! We're not covering this edge case.

Or are we assuming (for now), that people installing the development branch are existing users and somewhat more experienced with how Plotman works? At the very least, #84 should be addressed before merging to main.

@altendky
Copy link
Collaborator

altendky commented Apr 6, 2021

It's not like I have some long standing involvement with plotman either. :] But anyways, good points. I'll add some wording both places on how to get a copy of the configuration for now. I'm good with also getting some configuration location changes in before a 'release' (I think that's what merging to main is?).

@jkbecker
Copy link
Contributor Author

jkbecker commented Apr 6, 2021

Hm, random idea: Maybe it would be a good idea to merge the current development branch progress to main, before merging setup into development?

I feel like there are a lot of small improvements in development that could be helpful now, people are complaining about bad GUI plot handling almost every other hour for example. Merging setup onto development would just make merging develop on main a much bigger change, right?

Copy link
Collaborator

@altendky altendky left a comment

Choose a reason for hiding this comment

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

The request for more commentary on config.yaml is worthy of being a requested change I think. Even though I plan on doing it.

#61 (comment)

@altendky altendky merged commit 5630f9c into ericaltendorf:development Apr 7, 2021
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.

None yet

3 participants