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

Broken Gantt visualization #8

Closed
bleuse opened this issue Sep 22, 2017 · 6 comments
Closed

Broken Gantt visualization #8

bleuse opened this issue Sep 22, 2017 · 6 comments
Assignees

Comments

@bleuse
Copy link
Collaborator

bleuse commented Sep 22, 2017

The commit 9e590c6 by @stlackner breaks the existing Gantt chart visualization. This is due to the fact that the introduced map_unique_numbers relies on the workload being a BatSim workload (more specifically on the column workload_name for example).

In my understanding of the project, evalys is meant to be a bit more generic than a visualization tool for BatSim. I understand however that BatSim hackers are the main contributors to this project.

I started a refactor of the visualization code (see branch refactor/visu) that should help to make visualization code support various versions.
@stlackner could you have a look, and tell me if your code is compatible with the new approach? If yes, I'd be glad to get some hints :) Moreover, if you have an example script I can use to test, it'll be awesome.

@stlackner
Copy link
Collaborator

I have indeed only tested it with a Batsim workload as I was not aware that some of the columns would be absent in other workloads. So thanks for pointing that out.

The new branch refactor/visu-merge-jobs is a re-implementation of the concept of map_unique_numbers with your refactored code. The implementation no longer relies on the workload_name since I can work around this by pre-processing the workloads before using them with evalys.

I have added two example workloads in examples/stlackner which should just work by plotting them with the command line interface. Please also test my changes with your workloads and let me know if something is still missing. :)

@bleuse
Copy link
Collaborator Author

bleuse commented Sep 28, 2017

Thanks for your input ;)

I have however completely changed the architecture of the visualization modules. The approach initially proposed introduced nasty quirks, and did not allow to reuse code that much.

I did not integrate your code in evalys.visu for the following reasons:

  • Your code does not manage the case where a job is segmented in non-contiguous time segments.
  • It is preferable to keep code under evalys as simple as possible, while letting users adapt it to their needs (via sub-classing for example).

I have nonetheless merged your work into examples/stlackner/segmented_gantt.py (see commits 4ff6823 and 06cff94), and you should be able to use plot_segmented_gantt almost out of the box.

Please also note that the CLI is deprecated, and its maintainance is not a priority.

@stlackner
Copy link
Collaborator

Thank you for pointing that out.

I agree that my use case was probably too specialised if you want to keep the main code of evalys as simple as possible. The example you have provided is also working without any problems, so thank you for that.

However, wouldn't it be better to remove the CLI then completely? I mean if it is deprecated and if we don't even try to keep it compatible with the legacy API and the CLI is instead completely broken with the new changes it probably makes more sense to remove the broken code.
To keep the code would only make sense if we at least try to maintain the code in such a way that no exceptions are thrown and that the simple examples are still working.

By the way, are you still working on the visualisation code currently or when will you merge it into master?

@bleuse
Copy link
Collaborator Author

bleuse commented Oct 16, 2017

I totally agree we should remove the CLI, and I opened an issue to keep that in mind (see #9).
Before I merge the visualization code, I still need to document the new architecture. I hope to do so by the end of the week.

@mickours
Copy link
Contributor

mickours commented Feb 1, 2018

@bleuse I have a user that have the same problem with workload name. Is your work on the refactoring usable right now? I'd like to merge it and make a release.

@bleuse
Copy link
Collaborator Author

bleuse commented Feb 1, 2018

I just pushed my work into master. Let me know if the new version is more usable.

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

No branches or pull requests

3 participants